diff --git a/docs/adr/27626-sandbox-agent-version-and-network-firewall-migration.md b/docs/adr/27626-sandbox-agent-version-and-network-firewall-migration.md new file mode 100644 index 00000000000..884dfa9810e --- /dev/null +++ b/docs/adr/27626-sandbox-agent-version-and-network-firewall-migration.md @@ -0,0 +1,76 @@ +# ADR-27626: Introduce sandbox.agent.version and Remove Deprecated network.firewall Field + +**Date**: 2026-04-21 +**Status**: Draft +**Deciders**: Unknown (copilot-swe-agent / pelikhan) + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +The `network.firewall` frontmatter field was previously used to configure the Agent Workflow Firewall (AWF) in agentic workflow definitions. It was deprecated in favor of the unified `sandbox.agent` configuration block, but the migration codemod only handled the `true` case, leaving `false`, `null`, `"disable"`, and object-with-version forms unmigrated. Additionally, there was no mechanism to pin a specific AWF version via frontmatter — users who needed to run a particular AWF release had no stable, documented way to express that constraint. This PR addresses both gaps simultaneously: removing the deprecated field from the schema and expanding the codemod to cover all value variants. + +### Decision + +We will remove `network.firewall` from the frontmatter schema entirely and add `sandbox.agent.version` as a first-class string field for pinning the AWF version used during installation and runtime. The codemod will be expanded to migrate all `network.firewall` value forms — `true`, `false`, `null`, `"disable"`, and `{version: ...}` objects — to their `sandbox.agent` equivalents. This consolidates AWF configuration under a single `sandbox.agent` surface and ensures the migration path covers every variant that appears in the wild. + +### Alternatives Considered + +#### Alternative 1: Retain network.firewall as a Deprecated Alias + +Keep `network.firewall` in the schema with a deprecation warning, parsing it alongside `sandbox.agent` and merging the values at runtime. This avoids a hard removal and gives teams more migration runway, but perpetuates two competing configuration surfaces indefinitely, increases parser complexity, and makes it harder to reason about precedence when both fields are set. + +#### Alternative 2: Introduce a Flat sandbox.awf-version Field + +Add a top-level sibling key `sandbox.awf-version` (or similar) rather than nesting the version under `sandbox.agent`. This is marginally more ergonomic to type but diverges from the `sandbox.agent` object model already established, creates a second place where AWF version information lives, and complicates the precedence rules for effective version resolution. + +### Consequences + +#### Positive +- All `network.firewall` value forms now have a deterministic, tested migration path to `sandbox.agent`. +- Users can pin an explicit AWF version via `sandbox.agent.version`, enabling reproducible builds without relying on the latest release. +- The schema surface is reduced by removing a deprecated field and its associated validation rules. + +#### Negative +- Behavioral change: `network.firewall: false` previously produced no `sandbox` block; it now migrates to `sandbox.agent: false`. Workflows relying on the old no-op behavior will see a new block added by the codemod. +- The `normalizeFirewallVersion` helper must handle all numeric YAML types (int8 through uint64, float32/float64) because YAML parsers may unmarshal numeric version values into any of these types, increasing codemod surface area. + +#### Neutral +- The codemod expansion requires updating existing tests that asserted `sandbox:` was *not* added for `false` and nested object cases; these expectations were inverted. +- The `aw-info` AWF version reporting now reads `sandbox.agent.version` in addition to the legacy `firewallVersion` field, requiring both paths to be tested. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Frontmatter Schema + +1. Implementations **MUST NOT** include `network.firewall` in the frontmatter schema as a valid, non-deprecated field. +2. Implementations **MUST** expose `sandbox.agent.version` as an optional string field in the frontmatter schema for specifying an AWF version override. +3. The `sandbox.agent.version` field **MUST** be treated as a string type; numeric-like values written in YAML **MUST** be quoted at generation time to prevent YAML parsers from interpreting them as numbers. + +### Codemod Migration + +1. The `network.firewall` codemod **MUST** produce a `sandbox.agent` block for every non-absent value of `network.firewall`, including `true`, `false`, `null`, `"disable"`, and object forms. +2. When `network.firewall` is `true` or `null`, the codemod **MUST** emit `sandbox.agent: awf`. +3. When `network.firewall` is `false` or `"disable"`, the codemod **MUST** emit `sandbox.agent: false`. +4. When `network.firewall` is an object containing a `version` key, the codemod **MUST** emit a `sandbox.agent` object block with `id: awf` and `version: ""`. +5. The codemod **MUST NOT** add a `sandbox` block when one already exists in the frontmatter. +6. Numeric version values encountered during migration **MUST** be normalized to their string representation before being written as `sandbox.agent.version`. + +### AWF Version Resolution + +1. When `sandbox.agent.version` is set, it **MUST** take precedence over any version derived from the legacy `network.firewall` configuration when resolving the effective AWF version for installation and runtime. +2. Implementations **SHOULD** surface the resolved AWF version in `aw-info` metadata so that workflow authors can verify which version is active. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24736713102) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/docs/src/content/docs/reference/frontmatter-full.md b/docs/src/content/docs/reference/frontmatter-full.md index a0032ecaf02..f700d66b2b6 100644 --- a/docs/src/content/docs/reference/frontmatter-full.md +++ b/docs/src/content/docs/reference/frontmatter-full.md @@ -1309,6 +1309,10 @@ sandbox: # (optional) type: "awf" + # AWF version override used to install and run the matching firewall version. + # (optional) + version: "example-value" + # Container mounts to add when using AWF. Each mount is specified using Docker # mount syntax: 'source:destination:mode' where mode can be 'ro' (read-only) or # 'rw' (read-write). Example: '/host/path:/container/path:ro' diff --git a/pkg/cli/codemod_network_firewall.go b/pkg/cli/codemod_network_firewall.go index 42cd153d8a1..395c109d0c5 100644 --- a/pkg/cli/codemod_network_firewall.go +++ b/pkg/cli/codemod_network_firewall.go @@ -1,6 +1,7 @@ package cli import ( + "strconv" "strings" "github.com/github/gh-aw/pkg/logger" @@ -20,53 +21,242 @@ func getNetworkFirewallCodemod() Codemod { LogMsg: "Applied network.firewall migration (firewall now always enabled via sandbox.agent: awf default)", Log: networkFirewallCodemodLog, PostTransform: func(lines []string, frontmatter map[string]any, fieldValue any) []string { - // Note: We no longer set sandbox.agent: false since the firewall is mandatory - // The firewall is always enabled via the default sandbox.agent: awf - _, hasSandbox := frontmatter["sandbox"] - // Add sandbox.agent if not already present AND if firewall was explicitly true - // (no need to add sandbox.agent: awf if firewall was false, since awf is now the default) - if !hasSandbox && fieldValue == true { - // Only add sandbox.agent: awf if firewall was explicitly set to true - sandboxLines := []string{ - "sandbox:", - " agent: awf # Firewall enabled (migrated from network.firewall)", + if !hasSandbox { + sandboxLines := sandboxAgentLinesFromFirewall(fieldValue) + if len(sandboxLines) > 0 { + lines = insertSandboxAfterNetworkBlock(lines, sandboxLines) + networkFirewallCodemodLog.Print("Converted deprecated network.firewall to sandbox.agent") } + return lines + } - // Try to place it after network block - insertIndex := -1 - inNet := false - for i, line := range lines { - trimmed := strings.TrimSpace(line) - if strings.HasPrefix(trimmed, "network:") { - inNet = true - } else if inNet && len(trimmed) > 0 { - // Check if this is a top-level key (no leading whitespace) - if isTopLevelKey(line) { - // Found next top-level key - insertIndex = i - break - } - } - } + lines, merged := mergeFirewallIntoExistingSandbox(lines, fieldValue) + if merged { + networkFirewallCodemodLog.Print("Merged deprecated network.firewall into existing sandbox.agent") + } + return lines + }, + }) +} - if insertIndex >= 0 { - // Insert after network block - newLines := make([]string, 0, len(lines)+len(sandboxLines)) - newLines = append(newLines, lines[:insertIndex]...) - newLines = append(newLines, sandboxLines...) - newLines = append(newLines, lines[insertIndex:]...) - lines = newLines - } else { - // Append at the end - lines = append(lines, sandboxLines...) +func sandboxAgentLinesFromFirewall(fieldValue any) []string { + switch value := fieldValue.(type) { + case bool: + if value { + return []string{ + "sandbox:", + " agent: awf # Migrated from deprecated network setting", + } + } + return []string{ + "sandbox:", + " agent: false # Migrated from deprecated network setting", + } + case string: + if strings.EqualFold(strings.TrimSpace(value), "disable") { + return []string{ + "sandbox:", + " agent: false # Migrated from deprecated network setting", + } + } + case map[string]any: + versionValue, hasVersion := value["version"] + if hasVersion { + if version, ok := normalizeFirewallVersion(versionValue); ok { + return []string{ + "sandbox:", + " agent:", + " id: awf # Migrated from deprecated network setting", + " version: " + formatSandboxVersionYAML(version), } + } + } + return []string{ + "sandbox:", + " agent: awf # Migrated from deprecated network setting", + } + case nil: + return []string{ + "sandbox:", + " agent: awf # Migrated from deprecated network setting", + } + } + return nil +} + +func normalizeFirewallVersion(versionValue any) (string, bool) { + switch value := versionValue.(type) { + case string: + trimmed := strings.TrimSpace(value) + return trimmed, trimmed != "" + case int: + return strconv.Itoa(value), true + case int8: + return strconv.FormatInt(int64(value), 10), true + case int16: + return strconv.FormatInt(int64(value), 10), true + case int32: + return strconv.FormatInt(int64(value), 10), true + case int64: + return strconv.FormatInt(value, 10), true + case uint: + return strconv.FormatUint(uint64(value), 10), true + case uint8: + return strconv.FormatUint(uint64(value), 10), true + case uint16: + return strconv.FormatUint(uint64(value), 10), true + case uint32: + return strconv.FormatUint(uint64(value), 10), true + case uint64: + return strconv.FormatUint(value, 10), true + case float32: + return strconv.FormatFloat(float64(value), 'f', -1, 32), true + case float64: + return strconv.FormatFloat(value, 'f', -1, 64), true + default: + return "", false + } +} + +func formatSandboxVersionYAML(version string) string { + // Always quote because sandbox.agent.version is a string field, and this prevents + // YAML from interpreting numeric-like versions as numbers. + return strconv.Quote(version) +} + +func insertSandboxAfterNetworkBlock(lines []string, sandboxLines []string) []string { + insertIndex := -1 + inNetworkBlock := false + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "network:") { + inNetworkBlock = true + continue + } + if inNetworkBlock && len(trimmed) > 0 && isTopLevelKey(line) { + insertIndex = i + break + } + } - networkFirewallCodemodLog.Print("Added sandbox.agent: awf (firewall was explicitly enabled)") + if insertIndex >= 0 { + newLines := make([]string, 0, len(lines)+len(sandboxLines)) + newLines = append(newLines, lines[:insertIndex]...) + newLines = append(newLines, sandboxLines...) + newLines = append(newLines, lines[insertIndex:]...) + return newLines + } + + return append(lines, sandboxLines...) +} + +func mergeFirewallIntoExistingSandbox(lines []string, fieldValue any) ([]string, bool) { + agentLines := sandboxAgentLinesForExistingSandbox(fieldValue) + if len(agentLines) == 0 { + return lines, false + } + + sandboxIdx := -1 + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if isTopLevelKey(line) && strings.HasPrefix(trimmed, "sandbox:") { + sandboxIdx = i + break + } + } + if sandboxIdx == -1 { + return lines, false + } + + sandboxIndent := getIndentation(lines[sandboxIdx]) + agentIndent := sandboxIndent + " " + sandboxEnd := len(lines) + for i := sandboxIdx + 1; i < len(lines); i++ { + if isTopLevelKey(lines[i]) { + sandboxEnd = i + break + } + } + + agentStart := -1 + for i := sandboxIdx + 1; i < sandboxEnd; i++ { + trimmed := strings.TrimSpace(lines[i]) + if strings.HasPrefix(trimmed, "agent:") && getIndentation(lines[i]) == agentIndent { + agentStart = i + break + } + } + + indentedAgentLines := indentLines(agentLines, agentIndent) + if agentStart == -1 { + newLines := make([]string, 0, len(lines)+len(indentedAgentLines)) + newLines = append(newLines, lines[:sandboxIdx+1]...) + newLines = append(newLines, indentedAgentLines...) + newLines = append(newLines, lines[sandboxIdx+1:]...) + return newLines, true + } + + agentEnd := agentStart + 1 + agentFieldIndent := getIndentation(lines[agentStart]) + for agentEnd < sandboxEnd { + trimmed := strings.TrimSpace(lines[agentEnd]) + if trimmed == "" { + agentEnd++ + continue + } + if strings.HasPrefix(trimmed, "#") { + if len(getIndentation(lines[agentEnd])) > len(agentFieldIndent) { + agentEnd++ + continue } + break + } + if len(getIndentation(lines[agentEnd])) > len(agentFieldIndent) { + agentEnd++ + continue + } + break + } - return lines - }, - }) + newLines := make([]string, 0, len(lines)-((agentEnd-agentStart)-len(indentedAgentLines))) + newLines = append(newLines, lines[:agentStart]...) + newLines = append(newLines, indentedAgentLines...) + newLines = append(newLines, lines[agentEnd:]...) + return newLines, true +} + +func sandboxAgentLinesForExistingSandbox(fieldValue any) []string { + switch value := fieldValue.(type) { + case bool: + if !value { + return []string{"agent: false # Migrated from deprecated network setting"} + } + case string: + if strings.EqualFold(strings.TrimSpace(value), "disable") { + return []string{"agent: false # Migrated from deprecated network setting"} + } + case map[string]any: + versionValue, hasVersion := value["version"] + if hasVersion { + if version, ok := normalizeFirewallVersion(versionValue); ok { + return []string{ + "agent:", + " id: awf # Migrated from deprecated network setting", + " version: " + formatSandboxVersionYAML(version), + } + } + } + } + + return nil +} + +func indentLines(lines []string, indent string) []string { + indented := make([]string, 0, len(lines)) + for _, line := range lines { + indented = append(indented, indent+line) + } + return indented } diff --git a/pkg/cli/codemod_network_firewall_test.go b/pkg/cli/codemod_network_firewall_test.go index 4ff84e9b17c..8856d237076 100644 --- a/pkg/cli/codemod_network_firewall_test.go +++ b/pkg/cli/codemod_network_firewall_test.go @@ -80,7 +80,8 @@ permissions: require.NoError(t, err) assert.True(t, applied) assert.NotContains(t, result, "firewall:", "Should remove firewall field") - assert.NotContains(t, result, "sandbox:", "Should not add sandbox for firewall: false") + assert.Contains(t, result, "sandbox:", "Should add sandbox block") + assert.Contains(t, result, "agent: false", "Should convert firewall false to sandbox.agent: false") } func TestNetworkFirewallCodemod_NoNetworkField(t *testing.T) { @@ -176,6 +177,77 @@ permissions: assert.Contains(t, result, "agent: custom", "Should preserve existing sandbox config") } +func TestNetworkFirewallCodemod_MigratesFirewallFalseIntoExistingSandbox(t *testing.T) { + codemod := getNetworkFirewallCodemod() + + content := `--- +on: workflow_dispatch +network: + firewall: false +sandbox: + mcp: true +--- + +# Test Workflow` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "network": map[string]any{ + "firewall": false, + }, + "sandbox": map[string]any{ + "mcp": true, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.NotContains(t, result, "firewall:", "Should remove firewall field") + assert.Contains(t, result, "sandbox:", "Should preserve existing sandbox block") + assert.Contains(t, result, "mcp: true", "Should preserve existing sandbox settings") + assert.Contains(t, result, "agent: false", "Should migrate firewall false to sandbox.agent: false") +} + +func TestNetworkFirewallCodemod_MigratesFirewallVersionIntoExistingSandbox(t *testing.T) { + codemod := getNetworkFirewallCodemod() + + content := `--- +on: workflow_dispatch +network: + firewall: + version: 0.9 +sandbox: + mcp: true +--- + +# Test Workflow` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "network": map[string]any{ + "firewall": map[string]any{ + "version": 0.9, + }, + }, + "sandbox": map[string]any{ + "mcp": true, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.NotContains(t, result, "firewall:", "Should remove firewall field") + assert.Contains(t, result, "sandbox:", "Should preserve existing sandbox block") + assert.Contains(t, result, "mcp: true", "Should preserve existing sandbox settings") + assert.Contains(t, result, "agent:", "Should add sandbox.agent object") + assert.Contains(t, result, "id: awf", "Should set sandbox.agent.id") + assert.Contains(t, result, `version: "0.9"`, "Should migrate firewall.version to sandbox.agent.version") +} + func TestNetworkFirewallCodemod_PreservesOtherNetworkFields(t *testing.T) { codemod := getNetworkFirewallCodemod() @@ -312,4 +384,70 @@ permissions: assert.NotContains(t, result, "firewall:") assert.NotContains(t, result, "enabled:") assert.NotContains(t, result, "strict:") + assert.Contains(t, result, "sandbox:", "Should add sandbox block") + assert.Contains(t, result, "agent: awf", "Should convert firewall object to sandbox.agent: awf") +} + +func TestNetworkFirewallCodemod_NullFirewallAddsSandboxAgent(t *testing.T) { + codemod := getNetworkFirewallCodemod() + + content := `--- +on: workflow_dispatch +network: + firewall: null +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "network": map[string]any{ + "firewall": nil, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.NotContains(t, result, "firewall:", "Should remove firewall field") + assert.Contains(t, result, "sandbox:", "Should add sandbox block") + assert.Contains(t, result, "agent: awf", "Should convert firewall null to sandbox.agent: awf") +} + +func TestNetworkFirewallCodemod_PreservesFirewallVersionInSandboxAgent(t *testing.T) { + codemod := getNetworkFirewallCodemod() + + content := `--- +on: workflow_dispatch +network: + firewall: + version: v1.2.3 +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "network": map[string]any{ + "firewall": map[string]any{ + "version": "v1.2.3", + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.NotContains(t, result, "firewall:", "Should remove firewall field") + assert.Contains(t, result, "sandbox:", "Should add sandbox block") + assert.Contains(t, result, "id: awf", "Should create sandbox.agent object") + assert.Contains(t, result, `version: "v1.2.3"`, "Should preserve firewall.version as sandbox.agent.version") +} + +func TestNormalizeFirewallVersion_Float32Uses32BitPrecision(t *testing.T) { + version, ok := normalizeFirewallVersion(float32(0.9)) + require.True(t, ok) + assert.Equal(t, "0.9", version) } diff --git a/pkg/cli/fix_command_test.go b/pkg/cli/fix_command_test.go index 8a70135203d..40087399b43 100644 --- a/pkg/cli/fix_command_test.go +++ b/pkg/cli/fix_command_test.go @@ -186,9 +186,11 @@ This is a test workflow. t.Error("Expected firewall field to be removed, but it still exists") } - // firewall: null should NOT add sandbox.agent (only true values do) - if strings.Contains(updatedStr, "sandbox:") { - t.Errorf("Expected sandbox field NOT to be added for null firewall, got:\n%s", updatedStr) + if !strings.Contains(updatedStr, "sandbox:") { + t.Errorf("Expected sandbox field to be added for null firewall, got:\n%s", updatedStr) + } + if !strings.Contains(updatedStr, "agent: awf") { + t.Errorf("Expected sandbox.agent: awf to be added for null firewall, got:\n%s", updatedStr) } } @@ -261,9 +263,14 @@ This is a test workflow. t.Error("Expected version field to be removed, but it still exists") } - // firewall with nested properties (non-boolean) should NOT add sandbox.agent - if strings.Contains(updatedStr, "sandbox:") { - t.Errorf("Expected sandbox field NOT to be added for nested firewall, got:\n%s", updatedStr) + if !strings.Contains(updatedStr, "sandbox:") { + t.Errorf("Expected sandbox field to be added for nested firewall, got:\n%s", updatedStr) + } + if !strings.Contains(updatedStr, "agent:") { + t.Errorf("Expected sandbox.agent object to be added for nested firewall, got:\n%s", updatedStr) + } + if !strings.Contains(updatedStr, `version: "v1.0.0"`) { + t.Errorf("Expected firewall version to be migrated to sandbox.agent.version, got:\n%s", updatedStr) } // Verify compilation works @@ -358,9 +365,8 @@ This workflow tests comment and empty line handling. t.Error("Expected comment within firewall block to be removed, but it still exists") } - // firewall with nested properties should NOT add sandbox.agent - if strings.Contains(updatedStr, "sandbox:") { - t.Errorf("Expected sandbox field NOT to be added for nested firewall, got:\n%s", updatedStr) + if !strings.Contains(updatedStr, "sandbox:") { + t.Errorf("Expected sandbox field to be added for nested firewall, got:\n%s", updatedStr) } // Verify other network fields are preserved diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 3112e4103dd..1d45fd44986 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -2877,13 +2877,6 @@ }, { "allowed": ["defaults", "python", "node", "*.example.com"] - }, - { - "allowed": ["api.openai.com", "*.github.com"], - "firewall": { - "version": "v1.0.0", - "log-level": "debug" - } } ], "oneOf": [ @@ -2913,65 +2906,6 @@ "description": "Domain name or ecosystem identifier to block. Supports wildcards like '*.example.com' (matches sub.example.com, deep.nested.example.com, and example.com itself) and ecosystem names like 'python', 'node'." }, "$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. Supported for copilot, claude, codex, gemini, and crush engines.", - "deprecated": true, - "x-deprecation-message": "The firewall is now always enabled. Use 'sandbox.agent' to configure the sandbox type.", - "oneOf": [ - { - "type": "null", - "description": "Enable AWF with default settings (equivalent to empty object)" - }, - { - "type": "boolean", - "description": "Enable (true) or explicitly disable (false) AWF firewall" - }, - { - "type": "string", - "enum": ["disable"], - "description": "Disable AWF firewall (triggers warning if allowed != *, error in strict mode if allowed is not * or engine does not support firewall)" - }, - { - "type": "object", - "description": "Custom AWF configuration with version and arguments", - "properties": { - "args": { - "type": "array", - "description": "Optional additional arguments to pass to AWF wrapper", - "items": { - "type": "string" - } - }, - "version": { - "type": ["string", "number"], - "description": "AWF version to use (empty = latest release). Can be a string (e.g., 'v1.0.0', 'latest') or number (e.g., 20, 3.11). Numeric values are automatically converted to strings at runtime.", - "examples": ["v1.0.0", "latest", 20, 3.11] - }, - "log-level": { - "type": "string", - "description": "AWF log level (default: info). Valid values: debug, info, warn, error", - "enum": ["debug", "info", "warn", "error"] - }, - "ssl-bump": { - "type": "boolean", - "description": "Enable SSL Bump for HTTPS content inspection. When enabled, AWF can filter HTTPS traffic by URL patterns instead of just domain names. This feature is specific to AWF. Default: false", - "default": false - }, - "allow-urls": { - "type": "array", - "description": "URL patterns to allow for HTTPS traffic (requires ssl-bump: true). Supports wildcards for flexible path matching. Must include https:// scheme. This feature is specific to AWF.", - "items": { - "type": "string", - "pattern": "^https://.*", - "description": "HTTPS URL pattern with optional wildcards (e.g., 'https://github.com/githubnext/*')" - }, - "examples": [["https://github.com/githubnext/*", "https://api.github.com/repos/*"]] - } - }, - "additionalProperties": false - } - ] } }, "additionalProperties": false @@ -3023,6 +2957,10 @@ "enum": ["awf"], "description": "Legacy: Sandbox type to use (use 'id' instead)" }, + "version": { + "type": "string", + "description": "AWF version override used to install and run the matching firewall version." + }, "command": { "type": "string", "x-internal": true, diff --git a/pkg/workflow/aw_info_versions_test.go b/pkg/workflow/aw_info_versions_test.go index 9c85cb105dc..23360cd212a 100644 --- a/pkg/workflow/aw_info_versions_test.go +++ b/pkg/workflow/aw_info_versions_test.go @@ -164,6 +164,7 @@ func TestAwfVersionInAwInfo(t *testing.T) { name string firewallEnabled bool firewallVersion string + agentVersion string expectedAwfVersion string description string }{ @@ -185,9 +186,18 @@ func TestAwfVersionInAwInfo(t *testing.T) { name: "Firewall disabled", firewallEnabled: false, firewallVersion: "", + agentVersion: "", expectedAwfVersion: "", description: "Should have empty awf_version when firewall is disabled", }, + { + name: "sandbox.agent.version overrides firewall version", + firewallEnabled: true, + firewallVersion: "", + agentVersion: "v0.30.1", + expectedAwfVersion: "v0.30.1", + description: "Should prefer sandbox.agent.version override", + }, } for _, tt := range tests { @@ -211,6 +221,14 @@ func TestAwfVersionInAwInfo(t *testing.T) { }, } } + if tt.agentVersion != "" { + workflowData.SandboxConfig = &SandboxConfig{ + Agent: &AgentSandboxConfig{ + Type: SandboxTypeAWF, + Version: tt.agentVersion, + }, + } + } var yaml strings.Builder compiler.generateCreateAwInfo(&yaml, workflowData, engine) diff --git a/pkg/workflow/compiler_permissions_test.go b/pkg/workflow/compiler_permissions_test.go index 4cbe7fabb4c..25a2577cf74 100644 --- a/pkg/workflow/compiler_permissions_test.go +++ b/pkg/workflow/compiler_permissions_test.go @@ -219,7 +219,7 @@ This is a test workflow with empty network permissions (deny all). } }) - t.Run("network with allowed domains and firewall enabled should use AWF", func(t *testing.T) { + t.Run("network with allowed domains should use AWF", func(t *testing.T) { testContent := `--- on: push strict: false @@ -227,7 +227,6 @@ engine: id: claude network: allowed: ["example.com", "api.github.com"] - firewall: true --- # Test Workflow @@ -254,7 +253,7 @@ This is a test workflow with explicit network permissions. // Should contain AWF wrapper with --allow-domains if !strings.Contains(string(lockContent), "sudo -E awf") { - t.Error("Should contain AWF wrapper with explicit network permissions and firewall: true") + t.Error("Should contain AWF wrapper with explicit network permissions") } if !strings.Contains(string(lockContent), "--allow-domains") { t.Error("Should contain --allow-domains flag in AWF command") diff --git a/pkg/workflow/compiler_yaml.go b/pkg/workflow/compiler_yaml.go index f3777d7c793..3e5b5bb8d31 100644 --- a/pkg/workflow/compiler_yaml.go +++ b/pkg/workflow/compiler_yaml.go @@ -722,12 +722,12 @@ func (c *Compiler) generateCreateAwInfo(yaml *strings.Builder, data *WorkflowDat firewallVersion := "" if data.NetworkPermissions != nil { allowedDomains = data.NetworkPermissions.Allowed - if data.NetworkPermissions.Firewall != nil { - firewallEnabled = data.NetworkPermissions.Firewall.Enabled - firewallVersion = data.NetworkPermissions.Firewall.Version - if firewallEnabled && firewallVersion == "" { - firewallVersion = string(constants.DefaultFirewallVersion) - } + } + if firewallConfig := getFirewallConfig(data); firewallConfig != nil { + firewallEnabled = firewallConfig.Enabled + firewallVersion = firewallConfig.Version + if firewallEnabled && firewallVersion == "" { + firewallVersion = string(constants.DefaultFirewallVersion) } } diff --git a/pkg/workflow/firewall.go b/pkg/workflow/firewall.go index a04060a36d5..73599f09fe6 100644 --- a/pkg/workflow/firewall.go +++ b/pkg/workflow/firewall.go @@ -65,9 +65,29 @@ func getFirewallConfig(workflowData *WorkflowData) *FirewallConfig { return nil } - // Check network.firewall configuration + agentConfig := getAgentConfig(workflowData) + agentVersion := "" + if agentConfig != nil { + agentVersion = agentConfig.Version + } + + // Check resolved firewall configuration from network permissions if workflowData.NetworkPermissions != nil && workflowData.NetworkPermissions.Firewall != nil { config := workflowData.NetworkPermissions.Firewall + if agentVersion != "" { + if config.Version == agentVersion { + return config + } + + overrideConfig := *config + overrideConfig.Version = agentVersion + if config.Version == "" { + firewallLog.Printf("Using sandbox.agent.version %s for firewall version", agentVersion) + } else { + firewallLog.Printf("Overriding firewall version %s with sandbox.agent.version %s", config.Version, agentVersion) + } + return &overrideConfig + } if firewallLog.Enabled() { firewallLog.Printf("Retrieved firewall config: enabled=%v, version=%s, logLevel=%s", config.Enabled, config.Version, config.LogLevel) @@ -75,6 +95,14 @@ func getFirewallConfig(workflowData *WorkflowData) *FirewallConfig { return config } + if agentVersion != "" { + firewallLog.Printf("Using sandbox.agent.version override: %s", agentVersion) + return &FirewallConfig{ + Enabled: isFirewallEnabled(workflowData), + Version: agentVersion, + } + } + return nil } diff --git a/pkg/workflow/firewall_default_enablement_test.go b/pkg/workflow/firewall_default_enablement_test.go index 479cbb320e2..32ed3cf9df6 100644 --- a/pkg/workflow/firewall_default_enablement_test.go +++ b/pkg/workflow/firewall_default_enablement_test.go @@ -192,7 +192,7 @@ func TestCopilotFirewallDefaultIntegration(t *testing.T) { } }) - t.Run("copilot workflow with explicit firewall:false does not include AWF", func(t *testing.T) { + t.Run("copilot workflow with sandbox.agent:false does not include AWF", func(t *testing.T) { frontmatter := map[string]any{ "on": "workflow_dispatch", "permissions": map[string]any{ @@ -200,8 +200,10 @@ func TestCopilotFirewallDefaultIntegration(t *testing.T) { }, "engine": "copilot", "network": map[string]any{ - "allowed": []any{"example.com"}, - "firewall": false, + "allowed": []any{"example.com"}, + }, + "sandbox": map[string]any{ + "agent": false, }, } @@ -221,28 +223,23 @@ func TestCopilotFirewallDefaultIntegration(t *testing.T) { t.Fatal("Expected network permissions to be extracted") } - // Verify firewall is explicitly disabled - if networkPerms.Firewall == nil { - t.Error("Expected firewall config to be present") + sandboxConfig := c.extractSandboxConfig(frontmatter) + if sandboxConfig == nil || sandboxConfig.Agent == nil { + t.Fatal("Expected sandbox.agent configuration to be extracted") } - - if networkPerms.Firewall.Enabled { - t.Error("Expected firewall.Enabled to be false") + if !sandboxConfig.Agent.Disabled { + t.Fatal("Expected sandbox.agent to be explicitly disabled") } // Enable firewall by default (should not override explicit config) - enableFirewallByDefaultForCopilot(engineConfig.ID, networkPerms, nil) - - // Verify firewall is still disabled - if networkPerms.Firewall.Enabled { - t.Error("Expected firewall to remain disabled when explicitly set to false") - } + enableFirewallByDefaultForCopilot(engineConfig.ID, networkPerms, sandboxConfig) // Create workflow data workflowData := &WorkflowData{ Name: "test-workflow", EngineConfig: engineConfig, NetworkPermissions: networkPerms, + SandboxConfig: sandboxConfig, } // Get installation steps diff --git a/pkg/workflow/firewall_log_level_test.go b/pkg/workflow/firewall_log_level_test.go index ba42dcf6e5f..0cc06821fc1 100644 --- a/pkg/workflow/firewall_log_level_test.go +++ b/pkg/workflow/firewall_log_level_test.go @@ -7,12 +7,12 @@ import ( "testing" ) -// TestFirewallLogLevelParsing tests that the log-level field is correctly parsed +// TestFirewallLogLevelParsing tests handling of deprecated network.firewall parsing. func TestFirewallLogLevelParsing(t *testing.T) { compiler := NewCompiler() compiler.SetSkipValidation(true) - t.Run("log-level is parsed from network.firewall object", func(t *testing.T) { + t.Run("network.firewall object is ignored during extraction", func(t *testing.T) { frontmatter := map[string]any{ "network": map[string]any{ "firewall": map[string]any{ @@ -26,95 +26,8 @@ func TestFirewallLogLevelParsing(t *testing.T) { t.Fatal("Network permissions should not be nil") } - if networkPerms.Firewall == nil { - t.Fatal("Firewall config should not be nil") - } - - if networkPerms.Firewall.LogLevel != "debug" { - t.Errorf("Expected log-level 'debug', got '%s'", networkPerms.Firewall.LogLevel) - } - }) - - t.Run("log-level defaults to empty string when not specified", func(t *testing.T) { - frontmatter := map[string]any{ - "network": map[string]any{ - "firewall": map[string]any{ - "version": "v1.0.0", - }, - }, - } - - networkPerms := compiler.extractNetworkPermissions(frontmatter) - if networkPerms == nil { - t.Fatal("Network permissions should not be nil") - } - - if networkPerms.Firewall == nil { - t.Fatal("Firewall config should not be nil") - } - - if networkPerms.Firewall.LogLevel != "" { - t.Errorf("Expected log-level to be empty string, got '%s'", networkPerms.Firewall.LogLevel) - } - }) - - t.Run("log-level works with other firewall fields", func(t *testing.T) { - frontmatter := map[string]any{ - "network": map[string]any{ - "firewall": map[string]any{ - "version": "v1.0.0", - "log-level": "info", - "args": []any{"--custom-arg"}, - }, - }, - } - - networkPerms := compiler.extractNetworkPermissions(frontmatter) - if networkPerms == nil { - t.Fatal("Network permissions should not be nil") - } - - if networkPerms.Firewall == nil { - t.Fatal("Firewall config should not be nil") - } - - if networkPerms.Firewall.LogLevel != "info" { - t.Errorf("Expected log-level 'info', got '%s'", networkPerms.Firewall.LogLevel) - } - - if networkPerms.Firewall.Version != "v1.0.0" { - t.Errorf("Expected version 'v1.0.0', got '%s'", networkPerms.Firewall.Version) - } - - if len(networkPerms.Firewall.Args) != 1 { - t.Errorf("Expected 1 arg, got %d", len(networkPerms.Firewall.Args)) - } - }) - - t.Run("different log-level values are parsed correctly", func(t *testing.T) { - logLevels := []string{"debug", "info", "warn", "error"} - - for _, level := range logLevels { - frontmatter := map[string]any{ - "network": map[string]any{ - "firewall": map[string]any{ - "log-level": level, - }, - }, - } - - networkPerms := compiler.extractNetworkPermissions(frontmatter) - if networkPerms == nil { - t.Fatalf("Network permissions should not be nil for log-level '%s'", level) - } - - if networkPerms.Firewall == nil { - t.Fatalf("Firewall config should not be nil for log-level '%s'", level) - } - - if networkPerms.Firewall.LogLevel != level { - t.Errorf("Expected log-level '%s', got '%s'", level, networkPerms.Firewall.LogLevel) - } + if networkPerms.Firewall != nil { + t.Fatalf("Expected network.firewall to be ignored, got: %+v", networkPerms.Firewall) } }) } diff --git a/pkg/workflow/firewall_version_pinning_test.go b/pkg/workflow/firewall_version_pinning_test.go index dacb807be51..8b30d6703ab 100644 --- a/pkg/workflow/firewall_version_pinning_test.go +++ b/pkg/workflow/firewall_version_pinning_test.go @@ -158,6 +158,49 @@ func TestCopilotEngineFirewallInstallation(t *testing.T) { } }) + t.Run("uses sandbox.agent.version when firewall version is not specified", func(t *testing.T) { + engine := NewCopilotEngine() + sandboxAgentVersion := "v0.30.2" + workflowData := &WorkflowData{ + Name: "test-workflow", + EngineConfig: &EngineConfig{ + ID: "copilot", + }, + NetworkPermissions: &NetworkPermissions{ + Firewall: &FirewallConfig{ + Enabled: true, + }, + }, + SandboxConfig: &SandboxConfig{ + Agent: &AgentSandboxConfig{ + Type: SandboxTypeAWF, + Version: sandboxAgentVersion, + }, + }, + } + + steps := engine.GetInstallationSteps(workflowData) + + var foundAWFStep bool + var awfStepStr string + for _, step := range steps { + stepStr := strings.Join(step, "\n") + if strings.Contains(stepStr, "Install AWF binary") { + foundAWFStep = true + awfStepStr = stepStr + break + } + } + + if !foundAWFStep { + t.Fatal("Expected to find AWF installation step when firewall is enabled") + } + + if !strings.Contains(awfStepStr, sandboxAgentVersion) { + t.Errorf("AWF installation step should pass sandbox.agent.version %s to script", sandboxAgentVersion) + } + }) + t.Run("does not include AWF installation when firewall disabled", func(t *testing.T) { engine := NewCopilotEngine() workflowData := &WorkflowData{ diff --git a/pkg/workflow/frontmatter_extraction_security.go b/pkg/workflow/frontmatter_extraction_security.go index 6ec9a1015be..a9f8ceeead3 100644 --- a/pkg/workflow/frontmatter_extraction_security.go +++ b/pkg/workflow/frontmatter_extraction_security.go @@ -23,7 +23,7 @@ func (c *Compiler) extractNetworkPermissions(frontmatter map[string]any) *Networ return nil } - // Handle object format: { allowed: [...], firewall: ... } or {} + // Handle object format: { allowed: [...], blocked: [...] } or {} if networkObj, ok := network.(map[string]any); ok { frontmatterExtractionSecurityLog.Printf("Network permissions object format with %d fields", len(networkObj)) permissions := &NetworkPermissions{ @@ -54,12 +54,6 @@ func (c *Compiler) extractNetworkPermissions(frontmatter map[string]any) *Networ } } - // Extract firewall configuration if present - if firewall, hasFirewall := networkObj["firewall"]; hasFirewall { - frontmatterExtractionSecurityLog.Print("Extracting firewall configuration") - permissions.Firewall = c.extractFirewallConfig(firewall) - } - // Empty object {} means no network access (empty allowed list) return permissions } @@ -272,6 +266,13 @@ func (c *Compiler) extractAgentSandboxConfig(agentVal any) *AgentSandboxConfig { } } + // Extract version (AWF version override) + if versionVal, hasVersion := agentObj["version"]; hasVersion { + if versionStr, ok := versionVal.(string); ok { + agentConfig.Version = versionStr + } + } + // Extract config for SRT if configVal, hasConfig := agentObj["config"]; hasConfig { agentConfig.Config = c.extractSRTConfig(configVal) diff --git a/pkg/workflow/frontmatter_extraction_security_test.go b/pkg/workflow/frontmatter_extraction_security_test.go index 4bc37cbdaf9..68aec9991fc 100644 --- a/pkg/workflow/frontmatter_extraction_security_test.go +++ b/pkg/workflow/frontmatter_extraction_security_test.go @@ -118,6 +118,21 @@ func TestExtractFirewallConfig(t *testing.T) { }) } +func TestExtractAgentSandboxConfigVersion(t *testing.T) { + compiler := &Compiler{} + + t.Run("extracts sandbox.agent.version from object format", func(t *testing.T) { + agentObj := map[string]any{ + "id": "awf", + "version": "v0.30.1", + } + + config := compiler.extractAgentSandboxConfig(agentObj) + require.NotNil(t, config, "Should extract agent sandbox config") + assert.Equal(t, "v0.30.1", config.Version, "Should extract sandbox.agent.version") + }) +} + // TestExtractMCPGatewayConfigPayloadFields tests extraction of payload-related fields // from MCP gateway frontmatter configuration func TestExtractMCPGatewayConfigPayloadFields(t *testing.T) { diff --git a/pkg/workflow/network_merge_edge_cases_test.go b/pkg/workflow/network_merge_edge_cases_test.go index ca36a581244..fd6fa1060ac 100644 --- a/pkg/workflow/network_merge_edge_cases_test.go +++ b/pkg/workflow/network_merge_edge_cases_test.go @@ -46,7 +46,6 @@ network: allowed: - github.com - api.github.com - firewall: true imports: - shared.md --- diff --git a/pkg/workflow/network_merge_import_test.go b/pkg/workflow/network_merge_import_test.go index c535247329f..f82a0a800a7 100644 --- a/pkg/workflow/network_merge_import_test.go +++ b/pkg/workflow/network_merge_import_test.go @@ -51,7 +51,6 @@ network: allowed: - defaults - github.com - firewall: true imports: - shared-network.md --- diff --git a/pkg/workflow/sandbox.go b/pkg/workflow/sandbox.go index 69b023b1e41..b4fc2cf90e0 100644 --- a/pkg/workflow/sandbox.go +++ b/pkg/workflow/sandbox.go @@ -44,6 +44,7 @@ type SandboxConfig struct { type AgentSandboxConfig struct { ID string `yaml:"id,omitempty"` // Agent ID: "awf" or "srt" (replaces Type in new object format) Type SandboxType `yaml:"type,omitempty"` // Sandbox type: "awf" or "srt" (legacy, use ID instead) + Version string `yaml:"version,omitempty"` // AWF version override used to install and run the matching firewall version Disabled bool `yaml:"-"` // True when agent is explicitly set to false (disables firewall). This is a runtime flag, not serialized to YAML. Config *SandboxRuntimeConfig `yaml:"config,omitempty"` // Custom SRT config (optional) Command string `yaml:"command,omitempty"` // Custom command to replace AWF or SRT installation diff --git a/pkg/workflow/sandbox_agent_false_test.go b/pkg/workflow/sandbox_agent_false_test.go index 46f5c031db1..bd8a914dc0e 100644 --- a/pkg/workflow/sandbox_agent_false_test.go +++ b/pkg/workflow/sandbox_agent_false_test.go @@ -157,8 +157,8 @@ Test workflow to verify default sandbox.agent behavior (awf). }) } -func TestNetworkFirewallDeprecationWarning(t *testing.T) { - t.Run("network.firewall compiles successfully (deprecated)", func(t *testing.T) { +func TestNetworkFirewallFrontmatterRejected(t *testing.T) { + t.Run("network.firewall is rejected by schema", func(t *testing.T) { // Create temp directory for test workflows workflowsDir := t.TempDir() @@ -172,7 +172,7 @@ strict: false on: workflow_dispatch --- -Test workflow to verify network.firewall still works (deprecated). + Test workflow to verify network.firewall is rejected. ` workflowPath := filepath.Join(workflowsDir, "test-firewall-deprecated.md") @@ -181,13 +181,14 @@ Test workflow to verify network.firewall still works (deprecated). t.Fatalf("Failed to write workflow file: %v", err) } - // Compile the workflow compiler := NewCompiler() - compiler.SetSkipValidation(true) - // The compilation should succeed (deprecated fields should still work) - if err := compiler.CompileWorkflow(workflowPath); err != nil { - t.Fatalf("Compilation failed: %v", err) + err = compiler.CompileWorkflow(workflowPath) + if err == nil { + t.Fatal("Expected compilation to fail for deprecated network.firewall frontmatter, but got nil error") + } + if !strings.Contains(err.Error(), "firewall") { + t.Fatalf("Expected error to reference firewall field, got: %v", err) } }) } diff --git a/pkg/workflow/sandbox_custom_agent_test.go b/pkg/workflow/sandbox_custom_agent_test.go index 1a00ef9830b..5aac673e419 100644 --- a/pkg/workflow/sandbox_custom_agent_test.go +++ b/pkg/workflow/sandbox_custom_agent_test.go @@ -127,7 +127,6 @@ strict: false network: allowed: - "example.com" - firewall: true sandbox: agent: id: awf @@ -202,7 +201,6 @@ strict: false network: allowed: - "example.com" - firewall: true sandbox: agent: type: awf diff --git a/pkg/workflow/strict_mode_permissions_validation.go b/pkg/workflow/strict_mode_permissions_validation.go index a164ac002d5..edd38ea0fac 100644 --- a/pkg/workflow/strict_mode_permissions_validation.go +++ b/pkg/workflow/strict_mode_permissions_validation.go @@ -182,7 +182,7 @@ func (c *Compiler) validateStrictFirewall(engineID string, networkPermissions *N // In strict mode, firewall MUST be enabled if networkPermissions.Firewall == nil || !networkPermissions.Firewall.Enabled { strictModeValidationLog.Printf("Firewall validation failed: firewall not enabled in strict mode") - return fmt.Errorf("strict mode: firewall must be enabled for %s engine with network restrictions. The firewall should be enabled by default, but if you've explicitly disabled it with 'network.firewall: false' or 'sandbox.agent: false', this is not allowed in strict mode for security reasons. See: https://github.github.com/gh-aw/reference/network/", engineID) + return fmt.Errorf("strict mode: firewall must be enabled for %s engine with network restrictions. The firewall should be enabled by default, but if you've explicitly disabled it with 'sandbox.agent: false', this is not allowed in strict mode for security reasons. See: https://github.github.com/gh-aw/reference/network/", engineID) } strictModeValidationLog.Printf("Firewall validation passed")