From e93821276925aa755ca254bf329c34ce081310a6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 28 Jul 2026 04:42:51 +0000 Subject: [PATCH] fix: support network.allowed-domains as alias for network.allowed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Support `network.allowed-domains` as a silent alias for `network.allowed` in `extractNetworkPermissions`, unioning both lists when both are present - Add `allowed-domains` as a deprecated valid field in the network object JSON schema so it does not fail strict schema validation - Add `getNetworkAllowedDomainsCodemod` codemod to rename `network.allowed-domains` → `network.allowed` (merging with any existing `network.allowed` entries) - Register the codemod in `GetAllCodemods` and update `expectedCodemodOrder` test - Update `.github/aw/network.md` to document the common naming confusion and show the correct `network.allowed` syntax Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/aw/network.md | 23 ++ .github/workflows/smoke-copilot-auto.lock.yml | 1 - pkg/cli/codemod_network_allowed_domains.go | 241 +++++++++++++++++ .../codemod_network_allowed_domains_test.go | 245 ++++++++++++++++++ pkg/cli/fix_codemods.go | 1 + pkg/cli/fix_codemods_test.go | 1 + pkg/parser/schemas/main_workflow_schema.json | 12 +- .../frontmatter_extraction_security.go | 19 +- 8 files changed, 534 insertions(+), 9 deletions(-) create mode 100644 pkg/cli/codemod_network_allowed_domains.go create mode 100644 pkg/cli/codemod_network_allowed_domains_test.go diff --git a/.github/aw/network.md b/.github/aw/network.md index b4aec46bd10..570ab79b633 100644 --- a/.github/aw/network.md +++ b/.github/aw/network.md @@ -104,6 +104,29 @@ These look like ecosystem identifiers but are **not recognised** — using them | `docker` | Docker Hub / GHCR | `containers` | | `localhost` | Loopback interface | `local` | +## Common Naming Mistake: `allowed-domains` vs `allowed` + +The top-level `network` block uses **`allowed`** — not `allowed-domains`: + +```yaml +# WRONG — network.allowed-domains does not exist +network: + allowed-domains: + - defaults + - github + +# CORRECT +network: + allowed: + - defaults + - github +``` + +The name `allowed-domains` belongs to a different section: `safe-outputs.allowed-domains` controls which +domains are permitted in safe-output URL content. Do not mix the two. + +Run `gh aw fix` to automatically rename `network.allowed-domains` to `network.allowed`. + ## Domain Pattern Rules - **Wildcard `*` requires a dot prefix**: `*.example.com` valid; bare `*` blocked (rejected outright in strict mode). diff --git a/.github/workflows/smoke-copilot-auto.lock.yml b/.github/workflows/smoke-copilot-auto.lock.yml index 80f7930c2f1..9cac699d23d 100644 --- a/.github/workflows/smoke-copilot-auto.lock.yml +++ b/.github/workflows/smoke-copilot-auto.lock.yml @@ -149,7 +149,6 @@ jobs: GH_AW_INFO_FIREWALL_TYPE: "squid" GH_AW_INFO_FRONTMATTER_EMOJI: "🌸" GH_AW_COMPILED_STRICT: "true" - GH_AW_INFO_MODEL_COSTS: '{"providers":{"github-copilot":{"models":{"auto":{"cost":{"input":"8.5e-07","output":"1.55e-06"}}}}}}' GH_AW_INFO_FEATURES: '{"gh-aw-detection":false}' uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: diff --git a/pkg/cli/codemod_network_allowed_domains.go b/pkg/cli/codemod_network_allowed_domains.go new file mode 100644 index 00000000000..949edbc53e0 --- /dev/null +++ b/pkg/cli/codemod_network_allowed_domains.go @@ -0,0 +1,241 @@ +package cli + +import ( + "fmt" + "strings" + + "github.com/github/gh-aw/pkg/logger" +) + +var networkAllowedDomainsCodemodLog = logger.New("cli:codemod_network_allowed_domains") + +// getNetworkAllowedDomainsCodemod creates a codemod that renames network.allowed-domains to network.allowed. +// Users sometimes write "allowed-domains" by analogy with "safe-outputs.allowed-domains", but the correct +// key under the network block is "allowed". This codemod merges the two lists when both are present. +func getNetworkAllowedDomainsCodemod() Codemod { + return Codemod{ + ID: "network-allowed-domains-rename", + Name: "Rename network.allowed-domains to network.allowed", + Description: "Renames the deprecated 'network.allowed-domains' key to 'network.allowed', merging with any existing 'network.allowed' entries.", + IntroducedIn: "0.1.0", + Apply: func(content string, frontmatter map[string]any) (string, bool, error) { + networkValue, hasNetwork := frontmatter["network"] + if !hasNetwork { + return content, false, nil + } + + networkMap, ok := networkValue.(map[string]any) + if !ok { + return content, false, nil + } + + if _, hasAllowedDomains := networkMap["allowed-domains"]; !hasAllowedDomains { + return content, false, nil + } + + networkAllowedDomainsCodemodLog.Print("network.allowed-domains detected, migrating to network.allowed") + + newContent, applied, err := applyFrontmatterLineTransform(content, func(lines []string) ([]string, bool) { + return renameNetworkAllowedDomains(lines) + }) + + if err != nil { + return content, false, err + } + if applied { + networkAllowedDomainsCodemodLog.Print("Applied network.allowed-domains -> network.allowed migration") + } + return newContent, applied, nil + }, + } +} + +// renameNetworkAllowedDomains performs a single-pass rename of network.allowed-domains to +// network.allowed in the YAML frontmatter lines. When both keys are present, the domains are +// merged (allowed first, allowed-domains second, deduped). +func renameNetworkAllowedDomains(lines []string) ([]string, bool) { + // Locate the network block + networkStart, networkEnd := findNetworkBlockBounds(lines) + if networkStart == -1 { + return lines, false + } + + networkLines := lines[networkStart : networkEnd+1] + + // Find allowed-domains and allowed blocks within network + adStart, adEnd := findFieldBlock(networkLines, "allowed-domains") + if adStart == -1 { + return lines, false + } + allowedStart, allowedEnd := findFieldBlock(networkLines, "allowed") + + networkIndent := getIndentation(networkLines[0]) + fieldIndent := networkIndent + " " + + // Collect domain items from allowed-domains + adDomains := collectListItems(networkLines[adStart : adEnd+1]) + + if allowedStart == -1 { + // No existing allowed — simply rename allowed-domains: → allowed: + result := make([]string, len(lines)) + copy(result, lines) + absADStart := networkStart + adStart + result[absADStart] = fieldIndent + "allowed:" + networkAllowedDomainsCodemodLog.Printf("Renamed allowed-domains to allowed at line %d", absADStart+1) + return result, true + } + + // Both exist — merge and rewrite. Collect existing allowed domains. + existingDomains := collectListItems(networkLines[allowedStart : allowedEnd+1]) + + // Merge: existing first, then new (dedup) + merged := mergeDomains(existingDomains, adDomains) + + // Build new network block: remove both fields, insert merged allowed + newNetworkLines := removeBlock(networkLines, adStart, adEnd) + allowedStart2, allowedEnd2 := findFieldBlock(newNetworkLines, "allowed") + if allowedStart2 != -1 { + newNetworkLines = removeBlock(newNetworkLines, allowedStart2, allowedEnd2) + } + + // Insert merged allowed after network: header + mergedLines := buildAllowedLines(fieldIndent, merged) + newNetworkLines = insertAfterIndex(newNetworkLines, 0, mergedLines) + + result := make([]string, 0, len(lines)) + result = append(result, lines[:networkStart]...) + result = append(result, newNetworkLines...) + result = append(result, lines[networkEnd+1:]...) + networkAllowedDomainsCodemodLog.Printf("Merged allowed-domains into allowed (%d domain(s))", len(merged)) + return result, true +} + +// findNetworkBlockBounds returns the start and end indices (inclusive) of the network block +// in the YAML frontmatter lines. Returns (-1, -1) if not found. +func findNetworkBlockBounds(lines []string) (int, int) { + start := -1 + indent := "" + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if isTopLevelKey(line) && strings.HasPrefix(trimmed, "network:") { + start = i + indent = getIndentation(line) + continue + } + if start != -1 { + if trimmed == "" { + continue + } + if len(getIndentation(line)) <= len(indent) && !strings.HasPrefix(trimmed, "#") { + return start, i - 1 + } + } + } + if start != -1 { + return start, len(lines) - 1 + } + return -1, -1 +} + +// findFieldBlock returns the start and end indices (inclusive) of a YAML field block +// (key + all nested list items / sub-keys) within a slice of lines. +// Returns (-1, -1) if the field is not found. +func findFieldBlock(lines []string, key string) (int, int) { + start := -1 + keyIndent := "" + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if trimmed == "" { + continue + } + if strings.HasPrefix(trimmed, key+":") { + start = i + keyIndent = getIndentation(line) + continue + } + if start != -1 { + if trimmed == "" { + continue + } + currentIndent := getIndentation(line) + if len(currentIndent) <= len(keyIndent) { + return start, i - 1 + } + } + } + if start != -1 { + return start, len(lines) - 1 + } + return -1, -1 +} + +// collectListItems extracts the string values from a YAML list block. +// It reads lines of the form " - value". +func collectListItems(lines []string) []string { + var result []string + for _, line := range lines { + trimmed := strings.TrimSpace(line) + if val, ok := strings.CutPrefix(trimmed, "- "); ok { + val = strings.Trim(val, `"'`) + if val != "" { + result = append(result, val) + } + } + } + return result +} + +// mergeDomains merges two domain slices, deduplicating while preserving order (base first). +func mergeDomains(base, extra []string) []string { + seen := make(map[string]bool, len(base)+len(extra)) + result := make([]string, 0, len(base)+len(extra)) + for _, d := range base { + if !seen[d] { + seen[d] = true + result = append(result, d) + } + } + for _, d := range extra { + if !seen[d] { + seen[d] = true + result = append(result, d) + } + } + return result +} + +// removeBlock removes lines[start:end+1] from a slice and returns the new slice. +func removeBlock(lines []string, start, end int) []string { + result := make([]string, 0, len(lines)-(end-start+1)) + result = append(result, lines[:start]...) + result = append(result, lines[end+1:]...) + return result +} + +// insertAfterIndex inserts insertion after lines[idx] and returns the new slice. +func insertAfterIndex(lines []string, idx int, insertion []string) []string { + result := make([]string, 0, len(lines)+len(insertion)) + result = append(result, lines[:idx+1]...) + result = append(result, insertion...) + result = append(result, lines[idx+1:]...) + return result +} + +// buildAllowedLines builds the "allowed:" YAML lines for the given indent and domain list. +func buildAllowedLines(indent string, domains []string) []string { + if len(domains) == 0 { + return []string{indent + "allowed: []"} + } + lines := make([]string, 0, 1+len(domains)) + lines = append(lines, indent+"allowed:") + itemIndent := indent + " " + for _, d := range domains { + // Quote domains that contain special characters (e.g. wildcards) + if strings.Contains(d, "*") { + lines = append(lines, fmt.Sprintf("%s- %q", itemIndent, d)) + } else { + lines = append(lines, fmt.Sprintf("%s- %s", itemIndent, d)) + } + } + return lines +} diff --git a/pkg/cli/codemod_network_allowed_domains_test.go b/pkg/cli/codemod_network_allowed_domains_test.go new file mode 100644 index 00000000000..0da86ae62d6 --- /dev/null +++ b/pkg/cli/codemod_network_allowed_domains_test.go @@ -0,0 +1,245 @@ +//go:build !integration + +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetNetworkAllowedDomainsCodemod(t *testing.T) { + codemod := getNetworkAllowedDomainsCodemod() + + assert.Equal(t, "network-allowed-domains-rename", codemod.ID) + assert.Equal(t, "Rename network.allowed-domains to network.allowed", codemod.Name) + assert.NotEmpty(t, codemod.Description) + require.NotNil(t, codemod.Apply) +} + +func TestNetworkAllowedDomainsCodemod_NoNetwork(t *testing.T) { + codemod := getNetworkAllowedDomainsCodemod() + + content := `--- +on: workflow_dispatch +permissions: + contents: read +--- + +# Test Workflow` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": map[string]any{"contents": "read"}, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} + +func TestNetworkAllowedDomainsCodemod_NoAllowedDomains(t *testing.T) { + codemod := getNetworkAllowedDomainsCodemod() + + content := `--- +on: workflow_dispatch +network: + allowed: + - defaults + - github +--- + +# Test Workflow` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "network": map[string]any{ + "allowed": []any{"defaults", "github"}, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} + +func TestNetworkAllowedDomainsCodemod_RenamesAllowedDomains(t *testing.T) { + codemod := getNetworkAllowedDomainsCodemod() + + content := `--- +on: workflow_dispatch +network: + allowed-domains: + - defaults + - github +--- + +# Test Workflow` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "network": map[string]any{ + "allowed-domains": []any{"defaults", "github"}, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err) + assert.True(t, applied) + assert.NotContains(t, result, "allowed-domains:", "Should remove allowed-domains field") + assert.Contains(t, result, "allowed:", "Should contain allowed field") + assert.Contains(t, result, "- defaults", "Should contain defaults domain") + assert.Contains(t, result, "- github", "Should contain github domain") +} + +func TestNetworkAllowedDomainsCodemod_MergesWithExistingAllowed(t *testing.T) { + codemod := getNetworkAllowedDomainsCodemod() + + content := `--- +on: workflow_dispatch +network: + allowed: + - defaults + allowed-domains: + - github + - api.example.com +--- + +# Test Workflow` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "network": map[string]any{ + "allowed": []any{"defaults"}, + "allowed-domains": []any{"github", "api.example.com"}, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err) + assert.True(t, applied) + assert.NotContains(t, result, "allowed-domains:", "Should remove allowed-domains field") + assert.Contains(t, result, "allowed:", "Should contain allowed field") + assert.Contains(t, result, "- defaults", "Should contain defaults") + assert.Contains(t, result, "- github", "Should contain github") + assert.Contains(t, result, "- api.example.com", "Should contain api.example.com") +} + +func TestNetworkAllowedDomainsCodemod_DeduplicatesEntries(t *testing.T) { + codemod := getNetworkAllowedDomainsCodemod() + + content := `--- +on: workflow_dispatch +network: + allowed: + - defaults + - github + allowed-domains: + - github + - python +--- + +# Test Workflow` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "network": map[string]any{ + "allowed": []any{"defaults", "github"}, + "allowed-domains": []any{"github", "python"}, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err) + assert.True(t, applied) + assert.NotContains(t, result, "allowed-domains:", "Should remove allowed-domains field") + assert.Contains(t, result, "- defaults") + assert.Contains(t, result, "- github") + assert.Contains(t, result, "- python") + + // Count occurrences of "github" in items — should not be duplicated + itemCount := 0 + for _, line := range splitNetworkLines(result) { + if line == " - github" || line == " - github" { + itemCount++ + } + } + assert.Equal(t, 1, itemCount, "github should not be duplicated after merge") +} + +func TestNetworkAllowedDomainsCodemod_WildcardDomainsQuoted(t *testing.T) { + codemod := getNetworkAllowedDomainsCodemod() + + content := `--- +on: workflow_dispatch +network: + allowed-domains: + - "*.example.com" +--- + +# Test Workflow` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "network": map[string]any{ + "allowed-domains": []any{"*.example.com"}, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err) + assert.True(t, applied) + assert.NotContains(t, result, "allowed-domains:") + assert.Contains(t, result, "allowed:") + assert.Contains(t, result, `"*.example.com"`, "Wildcard domain should be quoted") +} + +func TestNetworkAllowedDomainsCodemod_NetworkWithoutAllowedGetsNewField(t *testing.T) { + codemod := getNetworkAllowedDomainsCodemod() + + content := `--- +on: workflow_dispatch +network: + allowed-domains: + - defaults + - node +--- + +# Test Workflow` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "network": map[string]any{ + "allowed-domains": []any{"defaults", "node"}, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err) + assert.True(t, applied) + assert.NotContains(t, result, "allowed-domains:") + assert.Contains(t, result, "allowed:") + assert.Contains(t, result, "- defaults") + assert.Contains(t, result, "- node") +} + +// splitNetworkLines is a test helper to split content into lines. +func splitNetworkLines(s string) []string { + var lines []string + current := "" + for _, c := range s { + if c == '\n' { + lines = append(lines, current) + current = "" + } else { + current += string(c) + } + } + if current != "" { + lines = append(lines, current) + } + return lines +} diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index 9661e73a951..c2ed7353947 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -77,6 +77,7 @@ func GetAllCodemods() []Codemod { getTopLevelEnvSecretsGuidedErrorCodemod(), // Detect secrets in top-level env: and emit guided error getAssignToAgentDefaultAgentCodemod(), // Rename deprecated default-agent to name in assign-to-agent getPlaywrightDomainsToNetworkAllowedCodemod(), // Migrate tools.playwright.allowed_domains to network.allowed + getNetworkAllowedDomainsCodemod(), // Rename network.allowed-domains to network.allowed getExpiresIntegerToDayStringCodemod(), // Convert expires integer (days) to string with 'd' suffix getGitHubAppCodemod(), // Rename deprecated 'app' to 'github-app' getGitHubAppClientIDCodemod(), // Rename deprecated github-app.app-id to github-app.client-id diff --git a/pkg/cli/fix_codemods_test.go b/pkg/cli/fix_codemods_test.go index 43b05ae61a4..64664139192 100644 --- a/pkg/cli/fix_codemods_test.go +++ b/pkg/cli/fix_codemods_test.go @@ -216,6 +216,7 @@ func expectedCodemodOrder() []string { "top-level-env-secrets-guided-error", "assign-to-agent-default-agent-to-name", "playwright-allowed-domains-migration", + "network-allowed-domains-rename", "expires-integer-to-string", "app-to-github-app", "github-app-app-id-to-client-id", diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 1790466fa5f..1e56b8789e5 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -3345,6 +3345,16 @@ "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." + }, + "allowed-domains": { + "type": "array", + "description": "Deprecated alias for 'allowed'. Use 'allowed' instead. Will be auto-migrated by gh aw fix.", + "deprecated": true, + "items": { + "type": "string", + "description": "Domain name or ecosystem identifier." + }, + "$comment": "Accepted for backwards compatibility. gh aw fix migrates this to 'allowed'." } }, "additionalProperties": false @@ -13868,7 +13878,7 @@ }, "sessionId": { "type": "string", - "description": "Optional session identifier injected as the x-session-id request header and session_id body field on Copilot BYOK upstream requests. Maps to AWF_PROVIDER_SESSION_ID. Only set this field when your upstream supports it — strict OpenAI-compatible upstreams (e.g. Azure OpenAI) reject the unknown session_id body field with HTTP 400. Example: \"${{ github.run_id }}\"." + "description": "Optional session identifier injected as the x-session-id request header and session_id body field on Copilot BYOK upstream requests. Maps to AWF_PROVIDER_SESSION_ID. Only set this field when your upstream supports it \u2014 strict OpenAI-compatible upstreams (e.g. Azure OpenAI) reject the unknown session_id body field with HTTP 400. Example: \"${{ github.run_id }}\"." } }, "additionalProperties": false diff --git a/pkg/workflow/frontmatter_extraction_security.go b/pkg/workflow/frontmatter_extraction_security.go index 446c29afba4..f04f76901fd 100644 --- a/pkg/workflow/frontmatter_extraction_security.go +++ b/pkg/workflow/frontmatter_extraction_security.go @@ -30,17 +30,22 @@ func (c *Compiler) extractNetworkPermissions(frontmatter map[string]any) *Networ ExplicitlyDefined: true, } - // Extract allowed domains if present - if allowed, hasAllowed := networkObj["allowed"]; hasAllowed { - if allowedSlice, ok := allowed.([]any); ok { - for _, domain := range allowedSlice { - if domainStr, ok := domain.(string); ok { - permissions.Allowed = append(permissions.Allowed, domainStr) + // Extract allowed domains from "allowed" (canonical) and "allowed-domains" (deprecated alias). + // Both are unioned when present. "allowed-domains" is accepted for backwards compatibility; + // users who write it by analogy with safe-outputs.allowed-domains are silently supported + // until gh aw fix migrates the key to "allowed". + for _, key := range []string{"allowed", "allowed-domains"} { + if raw, exists := networkObj[key]; exists { + if slice, ok := raw.([]any); ok { + for _, domain := range slice { + if domainStr, ok := domain.(string); ok { + permissions.Allowed = append(permissions.Allowed, domainStr) + } } } - frontmatterExtractionSecurityLog.Printf("Extracted %d allowed domains", len(permissions.Allowed)) } } + frontmatterExtractionSecurityLog.Printf("Extracted %d allowed domains", len(permissions.Allowed)) if allowedInput, hasAllowedInput := networkObj["allowed-input"]; hasAllowedInput { if allowedInputBool, ok := allowedInput.(bool); ok {