diff --git a/.github/workflows/blog-auditor.lock.yml b/.github/workflows/blog-auditor.lock.yml index d78b575804..21d8549f4e 100644 --- a/.github/workflows/blog-auditor.lock.yml +++ b/.github/workflows/blog-auditor.lock.yml @@ -27,7 +27,7 @@ # Imports: # - shared/reporting.md # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"58779154a8d932ebe96c1f75b0815f357a607c25ffe37821778a7c19ca5ddee6"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"66768026184ff86c246a33b44a5f634faf08765ece0df117122fdcec92bef186"} name: "Blog Auditor" "on": @@ -678,11 +678,7 @@ jobs: ], "entrypointArgs": [ "--output-dir", - "/tmp/gh-aw/mcp-logs/playwright", - "--allowed-hosts", - "localhost,localhost:*,127.0.0.1,127.0.0.1:*,githubnext.com,www.githubnext.com", - "--allowed-origins", - "localhost;localhost:*;127.0.0.1;127.0.0.1:*;githubnext.com;www.githubnext.com" + "/tmp/gh-aw/mcp-logs/playwright" ], "mounts": ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"] }, diff --git a/.github/workflows/blog-auditor.md b/.github/workflows/blog-auditor.md index 72b27ebbc9..194a0cdf02 100644 --- a/.github/workflows/blog-auditor.md +++ b/.github/workflows/blog-auditor.md @@ -17,9 +17,6 @@ network: - www.githubnext.com tools: playwright: - allowed_domains: - - githubnext.com - - www.githubnext.com bash: - "date *" - "echo *" diff --git a/.github/workflows/cloclo.lock.yml b/.github/workflows/cloclo.lock.yml index 9a6afa3818..43dc4c3e91 100644 --- a/.github/workflows/cloclo.lock.yml +++ b/.github/workflows/cloclo.lock.yml @@ -918,11 +918,7 @@ jobs: ], "entrypointArgs": [ "--output-dir", - "/tmp/gh-aw/mcp-logs/playwright", - "--allowed-hosts", - "localhost,localhost:*,127.0.0.1,127.0.0.1:*", - "--allowed-origins", - "localhost;localhost:*;127.0.0.1;127.0.0.1:*" + "/tmp/gh-aw/mcp-logs/playwright" ], "mounts": ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"] }, diff --git a/.github/workflows/daily-multi-device-docs-tester.lock.yml b/.github/workflows/daily-multi-device-docs-tester.lock.yml index 3460770646..cd20e0ea0d 100644 --- a/.github/workflows/daily-multi-device-docs-tester.lock.yml +++ b/.github/workflows/daily-multi-device-docs-tester.lock.yml @@ -748,11 +748,7 @@ jobs: ], "entrypointArgs": [ "--output-dir", - "/tmp/gh-aw/mcp-logs/playwright", - "--allowed-hosts", - "localhost,localhost:*,127.0.0.1,127.0.0.1:*", - "--allowed-origins", - "localhost;localhost:*;127.0.0.1;127.0.0.1:*" + "/tmp/gh-aw/mcp-logs/playwright" ], "mounts": ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"] }, diff --git a/.github/workflows/docs-noob-tester.lock.yml b/.github/workflows/docs-noob-tester.lock.yml index 0cc2110402..b60ca73470 100644 --- a/.github/workflows/docs-noob-tester.lock.yml +++ b/.github/workflows/docs-noob-tester.lock.yml @@ -700,7 +700,7 @@ jobs: "type": "stdio", "container": "mcr.microsoft.com/playwright/mcp", "args": ["--init", "--network", "host", "--security-opt", "seccomp=unconfined", "--ipc=host"], - "entrypointArgs": ["--output-dir", "/tmp/gh-aw/mcp-logs/playwright", "--allowed-hosts", "localhost,localhost:*,127.0.0.1,127.0.0.1:*", "--allowed-origins", "localhost;localhost:*;127.0.0.1;127.0.0.1:*"], + "entrypointArgs": ["--output-dir", "/tmp/gh-aw/mcp-logs/playwright"], "mounts": ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"] }, "safeoutputs": { diff --git a/.github/workflows/slide-deck-maintainer.lock.yml b/.github/workflows/slide-deck-maintainer.lock.yml index e197e60127..bb360819e9 100644 --- a/.github/workflows/slide-deck-maintainer.lock.yml +++ b/.github/workflows/slide-deck-maintainer.lock.yml @@ -728,7 +728,7 @@ jobs: "type": "stdio", "container": "mcr.microsoft.com/playwright/mcp", "args": ["--init", "--network", "host", "--security-opt", "seccomp=unconfined", "--ipc=host"], - "entrypointArgs": ["--output-dir", "/tmp/gh-aw/mcp-logs/playwright", "--allowed-hosts", "localhost,localhost:*,127.0.0.1,127.0.0.1:*", "--allowed-origins", "localhost;localhost:*;127.0.0.1;127.0.0.1:*"], + "entrypointArgs": ["--output-dir", "/tmp/gh-aw/mcp-logs/playwright"], "mounts": ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"] }, "safeoutputs": { diff --git a/.github/workflows/smoke-claude.lock.yml b/.github/workflows/smoke-claude.lock.yml index 4f1557c176..f5df6f3a15 100644 --- a/.github/workflows/smoke-claude.lock.yml +++ b/.github/workflows/smoke-claude.lock.yml @@ -35,7 +35,7 @@ # # inlined-imports: true # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"0760cd559b98c6f8276c3493ecc0951fe720e85d5ff255f36b0a0937eab2e13a"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"ede2fec60fd2b3a5ef0b98f74ae9793996ccd5ad3755f6bf6c3aac7df7779e57"} name: "Smoke Claude" "on": @@ -2176,11 +2176,7 @@ jobs: ], "entrypointArgs": [ "--output-dir", - "/tmp/gh-aw/mcp-logs/playwright", - "--allowed-hosts", - "localhost,localhost:*,127.0.0.1,127.0.0.1:*,github.com", - "--allowed-origins", - "localhost;localhost:*;127.0.0.1;127.0.0.1:*;github.com" + "/tmp/gh-aw/mcp-logs/playwright" ], "mounts": ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"] }, diff --git a/.github/workflows/smoke-claude.md b/.github/workflows/smoke-claude.md index b6df85a3ae..854aeebd09 100644 --- a/.github/workflows/smoke-claude.md +++ b/.github/workflows/smoke-claude.md @@ -43,8 +43,6 @@ tools: github: toolsets: [repos, pull_requests] playwright: - allowed_domains: - - github.com edit: bash: - "*" diff --git a/.github/workflows/smoke-codex.lock.yml b/.github/workflows/smoke-codex.lock.yml index 71eaab1ad0..ee097e6497 100644 --- a/.github/workflows/smoke-codex.lock.yml +++ b/.github/workflows/smoke-codex.lock.yml @@ -28,7 +28,7 @@ # - shared/gh.md # - shared/reporting.md # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"293f48aa360ae9abdc689bd2311d25b615842594cee0998c3e9a0873a944dcdd"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"7d48bac6df7757e4815d5c43bf8bc9072daed18816f793b91d734c6b752705f2"} name: "Smoke Codex" "on": @@ -1121,11 +1121,7 @@ jobs: ] entrypointArgs = [ "--output-dir", - "/tmp/gh-aw/mcp-logs/playwright", - "--allowed-hosts", - "localhost;localhost:*;127.0.0.1;127.0.0.1:*", - "--allowed-origins", - "localhost;localhost:*;127.0.0.1;127.0.0.1:*" + "/tmp/gh-aw/mcp-logs/playwright" ] mounts = ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"] @@ -1183,11 +1179,7 @@ jobs: ], "entrypointArgs": [ "--output-dir", - "/tmp/gh-aw/mcp-logs/playwright", - "--allowed-hosts", - "localhost,localhost:*,127.0.0.1,127.0.0.1:*,github.com", - "--allowed-origins", - "localhost;localhost:*;127.0.0.1;127.0.0.1:*;github.com" + "/tmp/gh-aw/mcp-logs/playwright" ], "mounts": ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"] }, diff --git a/.github/workflows/smoke-codex.md b/.github/workflows/smoke-codex.md index 04bb48b290..3862f548b2 100644 --- a/.github/workflows/smoke-codex.md +++ b/.github/workflows/smoke-codex.md @@ -27,8 +27,6 @@ tools: cache-memory: true github: playwright: - allowed_domains: - - github.com edit: bash: - "*" diff --git a/.github/workflows/smoke-copilot-arm.lock.yml b/.github/workflows/smoke-copilot-arm.lock.yml index 44d4ece607..db2175c87f 100644 --- a/.github/workflows/smoke-copilot-arm.lock.yml +++ b/.github/workflows/smoke-copilot-arm.lock.yml @@ -29,7 +29,7 @@ # - shared/github-queries-safe-input.md # - shared/reporting.md # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"37b001c8ed42d31e14d9d48f1b65b7f7a93caa96f98984ccfad66d2dfed5048f"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"c48c80387bfa8c38c2b91401970d6a06abfb23653e68538f8f267ee55c46b7d8"} name: "Smoke Copilot ARM64" "on": @@ -1689,7 +1689,7 @@ jobs: "type": "stdio", "container": "mcr.microsoft.com/playwright/mcp", "args": ["--init", "--network", "host", "--security-opt", "seccomp=unconfined", "--ipc=host"], - "entrypointArgs": ["--output-dir", "/tmp/gh-aw/mcp-logs/playwright", "--allowed-hosts", "localhost,localhost:*,127.0.0.1,127.0.0.1:*,github.com", "--allowed-origins", "localhost;localhost:*;127.0.0.1;127.0.0.1:*;github.com"], + "entrypointArgs": ["--output-dir", "/tmp/gh-aw/mcp-logs/playwright"], "mounts": ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"] }, "safeinputs": { diff --git a/.github/workflows/smoke-copilot-arm.md b/.github/workflows/smoke-copilot-arm.md index c3c8751ac0..8b783ec1db 100644 --- a/.github/workflows/smoke-copilot-arm.md +++ b/.github/workflows/smoke-copilot-arm.md @@ -34,8 +34,6 @@ tools: - "*" github: playwright: - allowed_domains: - - github.com serena: languages: go: {} diff --git a/.github/workflows/smoke-copilot.lock.yml b/.github/workflows/smoke-copilot.lock.yml index 6c013b0ca8..a031036311 100644 --- a/.github/workflows/smoke-copilot.lock.yml +++ b/.github/workflows/smoke-copilot.lock.yml @@ -29,7 +29,7 @@ # - shared/github-queries-safe-input.md # - shared/reporting.md # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"eff6019dcb863cc372bc2bdf31d8211c01fc473daf0e9b55d6df532bd7846408"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"f84cb897ab3a37514906baf0de987adbf90160780c7d3fa2dff0b66603e93e23"} name: "Smoke Copilot" "on": @@ -1691,7 +1691,7 @@ jobs: "type": "stdio", "container": "mcr.microsoft.com/playwright/mcp", "args": ["--init", "--network", "host", "--security-opt", "seccomp=unconfined", "--ipc=host"], - "entrypointArgs": ["--output-dir", "/tmp/gh-aw/mcp-logs/playwright", "--allowed-hosts", "localhost,localhost:*,127.0.0.1,127.0.0.1:*,github.com", "--allowed-origins", "localhost;localhost:*;127.0.0.1;127.0.0.1:*;github.com"], + "entrypointArgs": ["--output-dir", "/tmp/gh-aw/mcp-logs/playwright"], "mounts": ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"] }, "safeinputs": { diff --git a/.github/workflows/smoke-copilot.md b/.github/workflows/smoke-copilot.md index 7f87c2fb4d..cda289dab9 100644 --- a/.github/workflows/smoke-copilot.md +++ b/.github/workflows/smoke-copilot.md @@ -34,8 +34,6 @@ tools: - "*" github: playwright: - allowed_domains: - - github.com serena: languages: go: {} diff --git a/.github/workflows/unbloat-docs.lock.yml b/.github/workflows/unbloat-docs.lock.yml index 460a80befc..27df091d33 100644 --- a/.github/workflows/unbloat-docs.lock.yml +++ b/.github/workflows/unbloat-docs.lock.yml @@ -845,10 +845,6 @@ jobs: "entrypointArgs": [ "--output-dir", "/tmp/gh-aw/mcp-logs/playwright", - "--allowed-hosts", - "localhost,localhost:*,127.0.0.1,127.0.0.1:*", - "--allowed-origins", - "localhost;localhost:*;127.0.0.1;127.0.0.1:*", "--viewport-size", "1920x1080" ], diff --git a/.github/workflows/weekly-editors-health-check.lock.yml b/.github/workflows/weekly-editors-health-check.lock.yml index 929dc13c5e..008eba7bc6 100644 --- a/.github/workflows/weekly-editors-health-check.lock.yml +++ b/.github/workflows/weekly-editors-health-check.lock.yml @@ -23,7 +23,7 @@ # # Checks that the workflow editors listed in the documentation are still valid, takes Playwright screenshots, and opens a PR to update the docs with preview images # -# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"cad98865f3289e7779bfc0a4c4aeff5021753065adda88d5836ac17f88512a49"} +# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"14d8bdeb32a4dc257f4ddd7b84ee9f5339b6369fc2521f4b8640cc1fa9ca22a7"} name: "Weekly Editors Health Check" "on": @@ -718,7 +718,7 @@ jobs: "type": "stdio", "container": "mcr.microsoft.com/playwright/mcp", "args": ["--init", "--network", "host", "--security-opt", "seccomp=unconfined", "--ipc=host"], - "entrypointArgs": ["--output-dir", "/tmp/gh-aw/mcp-logs/playwright", "--allowed-hosts", "localhost,localhost:*,127.0.0.1,127.0.0.1:*,ashleywolf.github.io,github.github.com,mossaka.github.io", "--allowed-origins", "localhost;localhost:*;127.0.0.1;127.0.0.1:*;ashleywolf.github.io;github.github.com;mossaka.github.io"], + "entrypointArgs": ["--output-dir", "/tmp/gh-aw/mcp-logs/playwright"], "mounts": ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"] }, "safeoutputs": { diff --git a/.github/workflows/weekly-editors-health-check.md b/.github/workflows/weekly-editors-health-check.md index ee30b6b3b5..ed08beed85 100644 --- a/.github/workflows/weekly-editors-health-check.md +++ b/.github/workflows/weekly-editors-health-check.md @@ -22,10 +22,6 @@ network: tools: playwright: - allowed_domains: - - ashleywolf.github.io - - mossaka.github.io - - github.github.com web-fetch: bash: - "curl*" diff --git a/pkg/cli/codemod_playwright_domains.go b/pkg/cli/codemod_playwright_domains.go new file mode 100644 index 0000000000..add870bcea --- /dev/null +++ b/pkg/cli/codemod_playwright_domains.go @@ -0,0 +1,190 @@ +package cli + +import ( + "strings" + + "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/sliceutil" +) + +var playwrightDomainsCodemodLog = logger.New("cli:codemod_playwright_domains") + +// getPlaywrightDomainsCodemod creates a codemod that migrates tools.playwright.allowed_domains +// to network.allowed. Network egress for Playwright is now controlled by the workflow firewall. +func getPlaywrightDomainsCodemod() Codemod { + return Codemod{ + ID: "playwright-allowed-domains-migration", + Name: "Migrate playwright allowed_domains to network.allowed", + Description: "Moves 'tools.playwright.allowed_domains' to top-level 'network.allowed'. Playwright egress is now controlled by the firewall.", + IntroducedIn: "0.9.0", + Apply: func(content string, frontmatter map[string]any) (string, bool, error) { + // Check if tools.playwright.allowed_domains exists + toolsValue, hasTools := frontmatter["tools"] + if !hasTools { + return content, false, nil + } + + toolsMap, ok := toolsValue.(map[string]any) + if !ok { + return content, false, nil + } + + playwrightValue, hasPlaywright := toolsMap["playwright"] + if !hasPlaywright { + return content, false, nil + } + + playwrightMap, ok := playwrightValue.(map[string]any) + if !ok { + return content, false, nil + } + + allowedDomainsValue, hasAllowedDomains := playwrightMap["allowed_domains"] + if !hasAllowedDomains { + return content, false, nil + } + + // Extract domains from allowed_domains + var domains []string + switch v := allowedDomainsValue.(type) { + case []any: + for _, item := range v { + if s, ok := item.(string); ok { + domains = append(domains, s) + } + } + case []string: + domains = v + case string: + domains = []string{v} + } + + // Parse frontmatter lines + frontmatterLines, markdown, err := parseFrontmatterLines(content) + if err != nil { + return content, false, err + } + + // Remove allowed_domains from the tools.playwright block + result, modified := removeFieldFromPlaywright(frontmatterLines, "allowed_domains") + if !modified { + return content, false, nil + } + + playwrightDomainsCodemodLog.Printf("Removed allowed_domains from tools.playwright (%d domain(s))", len(domains)) + + // Merge with existing network.allowed + existingNetworkValue, hasTopLevelNetwork := frontmatter["network"] + var existingAllowed []string + if hasTopLevelNetwork { + if existingNetworkMap, ok := existingNetworkValue.(map[string]any); ok { + if existingAllowedValue, hasExistingAllowed := existingNetworkMap["allowed"]; hasExistingAllowed { + switch allowed := existingAllowedValue.(type) { + case []any: + for _, domain := range allowed { + if domainStr, ok := domain.(string); ok { + existingAllowed = append(existingAllowed, domainStr) + } + } + case []string: + existingAllowed = append(existingAllowed, allowed...) + } + } + } + } + + mergedDomains := sliceutil.Deduplicate(append(existingAllowed, domains...)) + + if hasTopLevelNetwork { + result = updateNetworkAllowed(result, mergedDomains) + playwrightDomainsCodemodLog.Printf("Updated top-level network.allowed with %d domain(s)", len(mergedDomains)) + } else { + result = addTopLevelNetwork(result, mergedDomains) + playwrightDomainsCodemodLog.Printf("Added top-level network.allowed with %d domain(s)", len(mergedDomains)) + } + + newContent := reconstructContent(result, markdown) + return newContent, true, nil + }, + } +} + +// removeFieldFromPlaywright removes a field from the tools.playwright block (two-level nesting) +func removeFieldFromPlaywright(lines []string, fieldName string) ([]string, bool) { + var result []string + var modified bool + var inTools bool + var toolsIndent string + var inPlaywright bool + var playwrightIndent string + var inFieldBlock bool + var fieldIndent string + + for i, line := range lines { + trimmed := strings.TrimSpace(line) + + // Track if we're in the tools block + if strings.HasPrefix(trimmed, "tools:") { + inTools = true + toolsIndent = getIndentation(line) + result = append(result, line) + continue + } + + // Check if we've left the tools block + if inTools && len(trimmed) > 0 && !strings.HasPrefix(trimmed, "#") { + if hasExitedBlock(line, toolsIndent) { + inTools = false + inPlaywright = false + } + } + + // Track if we're in the playwright block within tools + if inTools && strings.HasPrefix(trimmed, "playwright:") { + inPlaywright = true + playwrightIndent = getIndentation(line) + result = append(result, line) + continue + } + + // Check if we've left the playwright block + if inPlaywright && len(trimmed) > 0 && !strings.HasPrefix(trimmed, "#") { + if hasExitedBlock(line, playwrightIndent) { + inPlaywright = false + } + } + + // Remove the target field if in playwright block + if inPlaywright && strings.HasPrefix(trimmed, fieldName+":") { + modified = true + inFieldBlock = true + fieldIndent = getIndentation(line) + playwrightDomainsCodemodLog.Printf("Removed %s from tools.playwright on line %d", fieldName, i+1) + continue + } + + // Skip nested properties under the removed field + if inFieldBlock { + if len(trimmed) == 0 { + continue + } + currentIndent := getIndentation(line) + if strings.HasPrefix(trimmed, "#") { + if len(currentIndent) > len(fieldIndent) { + continue + } + inFieldBlock = false + result = append(result, line) + continue + } + if len(currentIndent) > len(fieldIndent) { + continue + } + inFieldBlock = false + } + + result = append(result, line) + } + + return result, modified +} diff --git a/pkg/cli/codemod_playwright_domains_test.go b/pkg/cli/codemod_playwright_domains_test.go new file mode 100644 index 0000000000..772706a8be --- /dev/null +++ b/pkg/cli/codemod_playwright_domains_test.go @@ -0,0 +1,285 @@ +//go:build !integration + +package cli + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetPlaywrightDomainsCodemod(t *testing.T) { + codemod := getPlaywrightDomainsCodemod() + + assert.Equal(t, "playwright-allowed-domains-migration", codemod.ID, "Codemod ID should match") + assert.Equal(t, "Migrate playwright allowed_domains to network.allowed", codemod.Name, "Codemod name should match") + assert.NotEmpty(t, codemod.Description, "Codemod should have a description") + assert.Equal(t, "0.9.0", codemod.IntroducedIn, "Codemod version should match") + require.NotNil(t, codemod.Apply, "Codemod should have an Apply function") +} + +func TestPlaywrightDomainsCodemod_NoTools(t *testing.T) { + codemod := getPlaywrightDomainsCodemod() + + content := `--- +on: workflow_dispatch +permissions: + contents: read +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err, "Apply should not return an error") + assert.False(t, applied, "Should not apply when no tools block") + assert.Equal(t, content, result, "Content should be unchanged") +} + +func TestPlaywrightDomainsCodemod_NoPlaywright(t *testing.T) { + codemod := getPlaywrightDomainsCodemod() + + content := `--- +on: workflow_dispatch +tools: + github: + mode: remote +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "tools": map[string]any{ + "github": map[string]any{"mode": "remote"}, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err, "Apply should not return an error") + assert.False(t, applied, "Should not apply when no playwright tool") + assert.Equal(t, content, result, "Content should be unchanged") +} + +func TestPlaywrightDomainsCodemod_NoAllowedDomains(t *testing.T) { + codemod := getPlaywrightDomainsCodemod() + + content := `--- +on: workflow_dispatch +tools: + playwright: + version: v1.41.0 +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "tools": map[string]any{ + "playwright": map[string]any{"version": "v1.41.0"}, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err, "Apply should not return an error") + assert.False(t, applied, "Should not apply when no allowed_domains") + assert.Equal(t, content, result, "Content should be unchanged") +} + +func TestPlaywrightDomainsCodemod_BasicMigration(t *testing.T) { + codemod := getPlaywrightDomainsCodemod() + + content := `--- +on: workflow_dispatch +tools: + playwright: + allowed_domains: + - github.com + - api.github.com +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "tools": map[string]any{ + "playwright": map[string]any{ + "allowed_domains": []any{"github.com", "api.github.com"}, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err, "Apply should not return an error") + assert.True(t, applied, "Codemod should report changes") + assert.NotContains(t, result, "allowed_domains", "Result should not contain allowed_domains") + assert.Contains(t, result, "network:", "Result should contain top-level network") + assert.Contains(t, result, "allowed:", "Result should contain network.allowed") + assert.Contains(t, result, "github.com", "Result should contain github.com domain") + assert.Contains(t, result, "api.github.com", "Result should contain api.github.com domain") +} + +func TestPlaywrightDomainsCodemod_PreservesVersion(t *testing.T) { + codemod := getPlaywrightDomainsCodemod() + + content := `--- +on: workflow_dispatch +tools: + playwright: + version: v1.41.0 + allowed_domains: + - example.com +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "tools": map[string]any{ + "playwright": map[string]any{ + "version": "v1.41.0", + "allowed_domains": []any{"example.com"}, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err, "Apply should not return an error") + assert.True(t, applied, "Codemod should report changes") + assert.NotContains(t, result, "allowed_domains", "Result should not contain allowed_domains") + assert.Contains(t, result, "playwright:", "Result should preserve playwright block") + assert.Contains(t, result, "version: v1.41.0", "Result should preserve version field") + assert.Contains(t, result, "network:", "Result should contain top-level network") + assert.Contains(t, result, "example.com", "Result should contain example.com domain") +} + +func TestPlaywrightDomainsCodemod_MergesWithExistingNetwork(t *testing.T) { + codemod := getPlaywrightDomainsCodemod() + + content := `--- +on: workflow_dispatch +tools: + playwright: + allowed_domains: + - example.com +network: + allowed: + - python + - existing.com +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "tools": map[string]any{ + "playwright": map[string]any{ + "allowed_domains": []any{"example.com"}, + }, + }, + "network": map[string]any{ + "allowed": []any{"python", "existing.com"}, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err, "Apply should not return an error") + assert.True(t, applied, "Codemod should report changes") + assert.NotContains(t, result, "allowed_domains", "Result should not contain allowed_domains") + assert.Contains(t, result, "python", "Result should preserve existing network domain") + assert.Contains(t, result, "existing.com", "Result should preserve existing.com domain") + assert.Contains(t, result, "example.com", "Result should add example.com domain") +} + +func TestPlaywrightDomainsCodemod_DeduplicatesDomains(t *testing.T) { + codemod := getPlaywrightDomainsCodemod() + + content := `--- +on: workflow_dispatch +tools: + playwright: + allowed_domains: + - github.com + - github.com +network: + allowed: + - github.com +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "tools": map[string]any{ + "playwright": map[string]any{ + "allowed_domains": []any{"github.com", "github.com"}, + }, + }, + "network": map[string]any{ + "allowed": []any{"github.com"}, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err, "Apply should not return an error") + assert.True(t, applied, "Codemod should report changes") + assert.NotContains(t, result, "allowed_domains", "Result should not contain allowed_domains") + // Count occurrences of github.com in the allowed block + count := 0 + inAllowed := false + for _, line := range splitLines(result) { + if line == " allowed:" { + inAllowed = true + continue + } + if inAllowed && len(line) > 0 && line[0] != ' ' { + inAllowed = false + } + if inAllowed && strings.Contains(line, "github.com") { + count++ + } + } + assert.Equal(t, 1, count, "github.com should appear exactly once in network.allowed") +} + +func TestPlaywrightDomainsCodemod_SingleDomainString(t *testing.T) { + codemod := getPlaywrightDomainsCodemod() + + content := `--- +on: workflow_dispatch +tools: + playwright: + allowed_domains: example.com +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "tools": map[string]any{ + "playwright": map[string]any{ + "allowed_domains": "example.com", + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err, "Apply should not return an error") + assert.True(t, applied, "Codemod should report changes") + assert.NotContains(t, result, "allowed_domains", "Result should not contain allowed_domains") + assert.Contains(t, result, "network:", "Result should contain network section") + assert.Contains(t, result, "example.com", "Result should contain example.com domain") +} diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index f2c8bd3b0d..be588a925e 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -40,5 +40,6 @@ func GetAllCodemods() []Codemod { getBotsToOnBotsCodemod(), // Move top-level bots to on.bots getEngineStepsToTopLevelCodemod(), // Move engine.steps to top-level steps getAssignToAgentDefaultAgentCodemod(), // Rename deprecated default-agent to name in assign-to-agent + getPlaywrightDomainsCodemod(), // Migrate tools.playwright.allowed_domains to network.allowed } } diff --git a/pkg/cli/fix_codemods_test.go b/pkg/cli/fix_codemods_test.go index 391b6d55fd..5331146199 100644 --- a/pkg/cli/fix_codemods_test.go +++ b/pkg/cli/fix_codemods_test.go @@ -43,7 +43,7 @@ func TestGetAllCodemods_ReturnsAllCodemods(t *testing.T) { codemods := GetAllCodemods() // Verify we have the expected number of codemods - expectedCount := 22 + expectedCount := 23 assert.Len(t, codemods, expectedCount, "Should return all %d codemods", expectedCount) // Verify all codemods have required fields @@ -126,6 +126,7 @@ func TestGetAllCodemods_InExpectedOrder(t *testing.T) { "bots-to-on-bots", "engine-steps-to-top-level", "assign-to-agent-default-agent-to-name", + "playwright-allowed-domains-migration", } require.Len(t, codemods, len(expectedOrder), "Should have expected number of codemods") diff --git a/pkg/cli/mcp_inspect_playwright_integration_test.go b/pkg/cli/mcp_inspect_playwright_integration_test.go index 9d41e21869..140069d1ff 100644 --- a/pkg/cli/mcp_inspect_playwright_integration_test.go +++ b/pkg/cli/mcp_inspect_playwright_integration_test.go @@ -26,30 +26,21 @@ func TestMCPInspectPlaywrightIntegration(t *testing.T) { name: "copilot", engineConfig: `engine: copilot tools: - playwright: - allowed_domains: - - "localhost" - - "example.com"`, + playwright:`, expectedSuccess: true, }, { name: "claude", engineConfig: `engine: claude tools: - playwright: - allowed_domains: - - "localhost" - - "example.com"`, + playwright:`, expectedSuccess: true, }, { name: "codex", engineConfig: `engine: codex tools: - playwright: - allowed_domains: - - "localhost" - - "example.com"`, + playwright:`, expectedSuccess: true, }, } @@ -109,11 +100,6 @@ This workflow tests playwright tool configuration. if strings.Contains(outputStr, "MCP configuration validation passed") { t.Logf("✓ MCP configuration validation passed for %s engine", tc.name) } - - // Verify allowed_domains appear in the output - if strings.Contains(outputStr, "localhost") || strings.Contains(outputStr, "example.com") { - t.Logf("✓ Allowed domains configuration detected for %s engine", tc.name) - } } }) } @@ -133,8 +119,6 @@ permissions: engine: copilot tools: playwright: - allowed_domains: - - "localhost" --- # Test Playwright Tools diff --git a/pkg/cli/mcp_inspect_playwright_live_integration_test.go b/pkg/cli/mcp_inspect_playwright_live_integration_test.go index 09367589f5..889d861831 100644 --- a/pkg/cli/mcp_inspect_playwright_live_integration_test.go +++ b/pkg/cli/mcp_inspect_playwright_live_integration_test.go @@ -77,28 +77,19 @@ func TestMCPInspectPlaywrightLiveIntegration(t *testing.T) { name: "copilot", engineConfig: `engine: copilot tools: - playwright: - allowed_domains: - - "localhost" - - "127.0.0.1"`, + playwright:`, }, { name: "claude", engineConfig: `engine: claude tools: - playwright: - allowed_domains: - - "localhost" - - "127.0.0.1"`, + playwright:`, }, { name: "codex", engineConfig: `engine: codex tools: - playwright: - allowed_domains: - - "localhost" - - "127.0.0.1"`, + playwright:`, }, } @@ -279,8 +270,6 @@ permissions: engine: copilot tools: playwright: - allowed_domains: - - "localhost" --- # Test Playwright with Docs Server diff --git a/pkg/cli/workflows/test-claude-playwright-accessibility-contrast.md b/pkg/cli/workflows/test-claude-playwright-accessibility-contrast.md index 0c0db67ae9..026d7e5e95 100644 --- a/pkg/cli/workflows/test-claude-playwright-accessibility-contrast.md +++ b/pkg/cli/workflows/test-claude-playwright-accessibility-contrast.md @@ -6,7 +6,6 @@ permissions: engine: claude tools: playwright: - allowed_domains: ["github.com", "*.github.com"] safe-outputs: create-issue: title-prefix: "[Accessibility] " diff --git a/pkg/cli/workflows/test-playwright-args.md b/pkg/cli/workflows/test-playwright-args.md index cade0161fd..acab2dfd32 100644 --- a/pkg/cli/workflows/test-playwright-args.md +++ b/pkg/cli/workflows/test-playwright-args.md @@ -10,7 +10,6 @@ safe-outputs: title-prefix: "[test] " tools: playwright: - allowed_domains: ["example.com"] args: ["--browser", "chromium"] --- @@ -19,7 +18,6 @@ tools: This workflow tests the `args` field for Playwright MCP server configuration. The workflow is configured with: -- `allowed_domains: ["example.com"]` to access example.com - `args: ["--browser", "chromium"]` to explicitly specify the browser Please perform the following tasks: diff --git a/pkg/parser/mcp.go b/pkg/parser/mcp.go index ccd38b602d..c8af77ff5c 100644 --- a/pkg/parser/mcp.go +++ b/pkg/parser/mcp.go @@ -438,7 +438,6 @@ func processBuiltinMCPTool(toolName string, toolValue any, serverFilter string) Command: "docker", Args: []string{ "run", "-i", "--rm", "--shm-size=2gb", "--cap-add=SYS_ADMIN", - "-e", "PLAYWRIGHT_ALLOWED_DOMAINS", "-v", "/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs", "mcr.microsoft.com/playwright:" + string(constants.DefaultPlaywrightBrowserVersion), }, @@ -447,34 +446,8 @@ func processBuiltinMCPTool(toolName string, toolValue any, serverFilter string) Name: "playwright", } - // Set default allowed domains to localhost with all port variations (matches implementation) - allowedDomains := constants.DefaultAllowedDomains - // Check for custom Playwright configuration if toolConfig, ok := toolValue.(map[string]any); ok { - // Handle allowed_domains configuration with bundle resolution - if domainsConfig, exists := toolConfig["allowed_domains"]; exists { - // For now, we'll use a simple conversion. In a full implementation, - // we'd need to use the same domain bundle resolution as the compiler - var customDomains []string - switch domains := domainsConfig.(type) { - case []string: - customDomains = domains - case []any: - customDomains = make([]string, len(domains)) - for i, domain := range domains { - if domainStr, ok := domain.(string); ok { - customDomains[i] = domainStr - } - } - case string: - customDomains = []string{domains} - } - - // Ensure localhost domains are always included - allowedDomains = EnsureLocalhostDomains(customDomains) - } - // Check for custom Docker image version if version, exists := toolConfig["version"]; exists { if versionStr := stringutil.ParseVersionValue(version); versionStr != "" { @@ -506,11 +479,6 @@ func processBuiltinMCPTool(toolName string, toolValue any, serverFilter string) } } - config.Env["PLAYWRIGHT_ALLOWED_DOMAINS"] = strings.Join(allowedDomains, ",") - if len(allowedDomains) == 0 { - config.Env["PLAYWRIGHT_BLOCK_ALL_DOMAINS"] = "true" - } - return &config, nil } else if toolName == "serena" { // Handle Serena MCP server - uses uvx to install and run from GitHub diff --git a/pkg/parser/mcp_test.go b/pkg/parser/mcp_test.go index 1a939bd119..40c73f533f 100644 --- a/pkg/parser/mcp_test.go +++ b/pkg/parser/mcp_test.go @@ -255,9 +255,7 @@ func TestExtractMCPConfigurations(t *testing.T) { name: "Playwright tool default configuration", frontmatter: map[string]any{ "tools": map[string]any{ - "playwright": map[string]any{ - "allowed_domains": []any{"github.com", "*.github.com"}, - }, + "playwright": map[string]any{}, }, }, expected: []MCPServerConfig{ @@ -265,11 +263,10 @@ func TestExtractMCPConfigurations(t *testing.T) { Command: "docker", Args: []string{ "run", "-i", "--rm", "--shm-size=2gb", "--cap-add=SYS_ADMIN", - "-e", "PLAYWRIGHT_ALLOWED_DOMAINS", "-v", "/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs", "mcr.microsoft.com/playwright:" + string(constants.DefaultPlaywrightBrowserVersion), }, - Env: map[string]string{"PLAYWRIGHT_ALLOWED_DOMAINS": "localhost,localhost:*,127.0.0.1,127.0.0.1:*,github.com,*.github.com"}}, Name: "playwright", + Env: map[string]string{}}, Name: "playwright", }, }, }, @@ -278,8 +275,7 @@ func TestExtractMCPConfigurations(t *testing.T) { frontmatter: map[string]any{ "tools": map[string]any{ "playwright": map[string]any{ - "allowed_domains": []any{"example.com"}, - "version": "v1.41.0", + "version": "v1.41.0", }, }, }, @@ -288,31 +284,10 @@ func TestExtractMCPConfigurations(t *testing.T) { Command: "docker", Args: []string{ "run", "-i", "--rm", "--shm-size=2gb", "--cap-add=SYS_ADMIN", - "-e", "PLAYWRIGHT_ALLOWED_DOMAINS", "-v", "/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs", "mcr.microsoft.com/playwright:v1.41.0", }, - Env: map[string]string{"PLAYWRIGHT_ALLOWED_DOMAINS": "localhost,localhost:*,127.0.0.1,127.0.0.1:*,example.com"}}, Name: "playwright", - }, - }, - }, - { - name: "Playwright tool with localhost default", - frontmatter: map[string]any{ - "tools": map[string]any{ - "playwright": map[string]any{}, - }, - }, - expected: []MCPServerConfig{ - {BaseMCPServerConfig: types.BaseMCPServerConfig{Type: "docker", - Command: "docker", - Args: []string{ - "run", "-i", "--rm", "--shm-size=2gb", "--cap-add=SYS_ADMIN", - "-e", "PLAYWRIGHT_ALLOWED_DOMAINS", - "-v", "/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs", - "mcr.microsoft.com/playwright:" + string(constants.DefaultPlaywrightBrowserVersion), - }, - Env: map[string]string{"PLAYWRIGHT_ALLOWED_DOMAINS": "localhost,localhost:*,127.0.0.1,127.0.0.1:*"}}, Name: "playwright", + Env: map[string]string{}}, Name: "playwright", }, }, }, @@ -321,8 +296,7 @@ func TestExtractMCPConfigurations(t *testing.T) { frontmatter: map[string]any{ "tools": map[string]any{ "playwright": map[string]any{ - "allowed_domains": []any{"example.com"}, - "version": 20, + "version": 20, }, }, }, @@ -331,11 +305,10 @@ func TestExtractMCPConfigurations(t *testing.T) { Command: "docker", Args: []string{ "run", "-i", "--rm", "--shm-size=2gb", "--cap-add=SYS_ADMIN", - "-e", "PLAYWRIGHT_ALLOWED_DOMAINS", "-v", "/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs", "mcr.microsoft.com/playwright:20", }, - Env: map[string]string{"PLAYWRIGHT_ALLOWED_DOMAINS": "localhost,localhost:*,127.0.0.1,127.0.0.1:*,example.com"}}, Name: "playwright", + Env: map[string]string{}}, Name: "playwright", }, }, }, @@ -344,8 +317,7 @@ func TestExtractMCPConfigurations(t *testing.T) { frontmatter: map[string]any{ "tools": map[string]any{ "playwright": map[string]any{ - "allowed_domains": []any{"example.com"}, - "version": 1.41, + "version": 1.41, }, }, }, @@ -354,11 +326,10 @@ func TestExtractMCPConfigurations(t *testing.T) { Command: "docker", Args: []string{ "run", "-i", "--rm", "--shm-size=2gb", "--cap-add=SYS_ADMIN", - "-e", "PLAYWRIGHT_ALLOWED_DOMAINS", "-v", "/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs", "mcr.microsoft.com/playwright:1.41", }, - Env: map[string]string{"PLAYWRIGHT_ALLOWED_DOMAINS": "localhost,localhost:*,127.0.0.1,127.0.0.1:*,example.com"}}, Name: "playwright", + Env: map[string]string{}}, Name: "playwright", }, }, }, @@ -367,8 +338,7 @@ func TestExtractMCPConfigurations(t *testing.T) { frontmatter: map[string]any{ "tools": map[string]any{ "playwright": map[string]any{ - "allowed_domains": []any{"example.com"}, - "version": int64(142), + "version": int64(142), }, }, }, @@ -377,11 +347,10 @@ func TestExtractMCPConfigurations(t *testing.T) { Command: "docker", Args: []string{ "run", "-i", "--rm", "--shm-size=2gb", "--cap-add=SYS_ADMIN", - "-e", "PLAYWRIGHT_ALLOWED_DOMAINS", "-v", "/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs", "mcr.microsoft.com/playwright:142", }, - Env: map[string]string{"PLAYWRIGHT_ALLOWED_DOMAINS": "localhost,localhost:*,127.0.0.1,127.0.0.1:*,example.com"}}, Name: "playwright", + Env: map[string]string{}}, Name: "playwright", }, }, }, diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 8e996d63f6..2fc0bbe0e6 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -3109,33 +3109,17 @@ "oneOf": [ { "type": "null", - "description": "Enable Playwright tool with default settings (localhost access only for security)" + "description": "Enable Playwright tool with default settings" }, { "type": "object", - "description": "Playwright tool configuration with custom version and domain restrictions", + "description": "Playwright tool configuration with custom version and arguments", "properties": { "version": { "type": ["string", "number"], "description": "Optional Playwright container version (e.g., 'v1.41.0', 1.41, 20). Numeric values are automatically converted to strings at runtime.", "examples": ["v1.41.0", 1.41, 20] }, - "allowed_domains": { - "description": "Domains allowed for Playwright browser network access. Defaults to localhost only for security.", - "oneOf": [ - { - "type": "array", - "description": "List of allowed domains or patterns (e.g., ['github.com', '*.example.com'])", - "items": { - "type": "string" - } - }, - { - "type": "string", - "description": "Single allowed domain (e.g., 'github.com')" - } - ] - }, "args": { "type": "array", "description": "Optional additional arguments to append to the generated MCP server command", diff --git a/pkg/workflow/args_field_test.go b/pkg/workflow/args_field_test.go index 85d099ef03..f61563bfdd 100644 --- a/pkg/workflow/args_field_test.go +++ b/pkg/workflow/args_field_test.go @@ -52,8 +52,7 @@ func TestArgsField(t *testing.T) { t.Run("Playwright args field extraction", func(t *testing.T) { // Test "args" field with []any playwrightTool := map[string]any{ - "allowed_domains": []any{"example.com"}, - "args": []any{"--browser", "firefox"}, + "args": []any{"--browser", "firefox"}, } playwrightConfig := parsePlaywrightTool(playwrightTool) result := getPlaywrightCustomArgs(playwrightConfig) @@ -66,8 +65,7 @@ func TestArgsField(t *testing.T) { // Test "args" field with []string playwrightToolString := map[string]any{ - "allowed_domains": []any{"example.com"}, - "args": []string{"--headless"}, + "args": []string{"--headless"}, } playwrightConfigString := parsePlaywrightTool(playwrightToolString) result = getPlaywrightCustomArgs(playwrightConfigString) @@ -79,9 +77,7 @@ func TestArgsField(t *testing.T) { } // Test no args field (default behavior) - playwrightToolDefault := map[string]any{ - "allowed_domains": []any{"example.com"}, - } + playwrightToolDefault := map[string]any{} playwrightConfigDefault := parsePlaywrightTool(playwrightToolDefault) result = getPlaywrightCustomArgs(playwrightConfigDefault) if result != nil { @@ -144,8 +140,7 @@ func TestArgsField(t *testing.T) { frontmatterPlaywright := map[string]any{ "tools": map[string]any{ "playwright": map[string]any{ - "allowed_domains": []any{"example.com"}, - "args": []any{"--browser", "firefox"}, + "args": []any{"--browser", "firefox"}, }, }, } diff --git a/pkg/workflow/codex_engine.go b/pkg/workflow/codex_engine.go index 34786ecfa8..19acf8572b 100644 --- a/pkg/workflow/codex_engine.go +++ b/pkg/workflow/codex_engine.go @@ -397,9 +397,8 @@ func (e *CodexEngine) expandNeutralToolsToCodexTools(toolsConfig *ToolsConfig) * if toolsConfig.Playwright != nil { // Create an updated Playwright config with the allowed tools playwrightConfig := &PlaywrightToolConfig{ - Version: toolsConfig.Playwright.Version, - AllowedDomains: toolsConfig.Playwright.AllowedDomains, - Args: toolsConfig.Playwright.Args, + Version: toolsConfig.Playwright.Version, + Args: toolsConfig.Playwright.Args, } result.Playwright = playwrightConfig @@ -411,9 +410,6 @@ func (e *CodexEngine) expandNeutralToolsToCodexTools(toolsConfig *ToolsConfig) * if playwrightConfig.Version != "" { playwrightMCP["version"] = playwrightConfig.Version } - if len(playwrightConfig.AllowedDomains) > 0 { - playwrightMCP["allowed_domains"] = playwrightConfig.AllowedDomains - } if len(playwrightConfig.Args) > 0 { playwrightMCP["args"] = playwrightConfig.Args } diff --git a/pkg/workflow/http_mcp_domains_test.go b/pkg/workflow/http_mcp_domains_test.go index 3441312ccf..e55841a589 100644 --- a/pkg/workflow/http_mcp_domains_test.go +++ b/pkg/workflow/http_mcp_domains_test.go @@ -61,9 +61,7 @@ func TestExtractHTTPMCPDomains(t *testing.T) { "type": "http", "url": "https://mcp.tavily.com/mcp/", }, - "playwright": map[string]any{ - "allowed_domains": []string{"github.com"}, - }, + "playwright": map[string]any{}, }, expected: []string{constants.GitHubCopilotMCPDomain, "mcp.tavily.com"}, }, @@ -244,9 +242,7 @@ func TestExtractPlaywrightDomains(t *testing.T) { { name: "playwright tool configured", tools: map[string]any{ - "playwright": map[string]any{ - "allowed_domains": []string{"github.com"}, - }, + "playwright": map[string]any{}, }, expected: []string{"playwright.download.prss.microsoft.com", "cdn.playwright.dev"}, }, @@ -305,9 +301,7 @@ func TestGetCopilotAllowedDomainsWithPlaywright(t *testing.T) { } tools := map[string]any{ - "playwright": map[string]any{ - "allowed_domains": []string{"github.com"}, - }, + "playwright": map[string]any{}, } result := GetCopilotAllowedDomainsWithTools(network, tools) @@ -325,9 +319,7 @@ func TestGetCodexAllowedDomainsWithPlaywright(t *testing.T) { } tools := map[string]any{ - "playwright": map[string]any{ - "allowed_domains": []string{"example.com"}, - }, + "playwright": map[string]any{}, } result := GetCodexAllowedDomainsWithTools(network, tools) diff --git a/pkg/workflow/importable_tools_test.go b/pkg/workflow/importable_tools_test.go index 1a24610ddb..34620cd2c8 100644 --- a/pkg/workflow/importable_tools_test.go +++ b/pkg/workflow/importable_tools_test.go @@ -26,9 +26,6 @@ description: "Shared playwright configuration" tools: playwright: version: "v1.41.0" - allowed_domains: - - "example.com" - - "github.com" network: allowed: - playwright @@ -85,14 +82,6 @@ Uses imported playwright tool. if !strings.Contains(workflowData, "mcr.microsoft.com/playwright/mcp") { t.Error("Expected compiled workflow to contain playwright Docker image") } - - // Verify allowed domains are present - if !strings.Contains(workflowData, "example.com") { - t.Error("Expected compiled workflow to contain example.com domain") - } - if !strings.Contains(workflowData, "github.com") { - t.Error("Expected compiled workflow to contain github.com domain") - } } // TestImportSerenaTool tests that serena tool can be imported from a shared workflow @@ -283,8 +272,6 @@ tools: - go playwright: version: "v1.41.0" - allowed_domains: - - "example.com" permissions: actions: read network: @@ -354,9 +341,6 @@ Uses all imported tools. if !strings.Contains(workflowData, "ghcr.io/github/serena-mcp-server:latest") { t.Error("Expected compiled workflow to contain serena Docker container") } - if !strings.Contains(workflowData, "example.com") { - t.Error("Expected compiled workflow to contain example.com domain for playwright") - } } // TestImportSerenaWithLanguageConfig tests serena with detailed language configuration @@ -452,8 +436,6 @@ description: "Shared playwright with custom args" tools: playwright: version: "v1.41.0" - allowed_domains: - - "example.com" args: - "--custom-flag" - "value" diff --git a/pkg/workflow/mcp_benchmark_test.go b/pkg/workflow/mcp_benchmark_test.go index 610800865d..29989435f6 100644 --- a/pkg/workflow/mcp_benchmark_test.go +++ b/pkg/workflow/mcp_benchmark_test.go @@ -10,8 +10,8 @@ import ( // BenchmarkRenderPlaywrightMCPConfig benchmarks Playwright MCP config generation func BenchmarkRenderPlaywrightMCPConfig(b *testing.B) { playwrightTool := map[string]any{ - "container": "mcr.microsoft.com/playwright:v1.41.0", - "allowed-domains": []any{"github.com", "*.github.io"}, + "container": "mcr.microsoft.com/playwright:v1.41.0", + "args": []any{"--debug"}, } playwrightConfig := parsePlaywrightTool(playwrightTool) @@ -26,12 +26,6 @@ func BenchmarkRenderPlaywrightMCPConfig(b *testing.B) { func BenchmarkGeneratePlaywrightDockerArgs(b *testing.B) { playwrightTool := map[string]any{ "container": "mcr.microsoft.com/playwright:v1.41.0", - "allowed-domains": []any{ - "github.com", - "*.github.io", - "api.github.com", - "*.googleapis.com", - }, } playwrightConfig := parsePlaywrightTool(playwrightTool) @@ -45,13 +39,7 @@ func BenchmarkGeneratePlaywrightDockerArgs(b *testing.B) { func BenchmarkRenderPlaywrightMCPConfig_Complex(b *testing.B) { playwrightTool := map[string]any{ "container": "mcr.microsoft.com/playwright:v1.41.0", - "allowed-domains": []any{ - "github.com", - "*.github.io", - "api.github.com", - "*.googleapis.com", - }, - "args": []any{"--debug", "--timeout", "30000"}, + "args": []any{"--debug", "--timeout", "30000"}, } playwrightConfig := parsePlaywrightTool(playwrightTool) @@ -64,16 +52,10 @@ func BenchmarkRenderPlaywrightMCPConfig_Complex(b *testing.B) { // BenchmarkExtractExpressionsFromPlaywrightArgs benchmarks expression extraction func BenchmarkExtractExpressionsFromPlaywrightArgs(b *testing.B) { - allowedDomains := []string{ - "github.com", - "*.github.io", - "${{ github.server_url }}", - "*.example.com", - } customArgs := []string{"--debug", "--timeout", "${{ github.event.inputs.timeout }}"} b.ResetTimer() for i := 0; i < b.N; i++ { - _ = extractExpressionsFromPlaywrightArgs(allowedDomains, customArgs) + _ = extractExpressionsFromPlaywrightArgs(customArgs) } } diff --git a/pkg/workflow/mcp_config_playwright_renderer.go b/pkg/workflow/mcp_config_playwright_renderer.go index f28d9f7c8a..294fdbb245 100644 --- a/pkg/workflow/mcp_config_playwright_renderer.go +++ b/pkg/workflow/mcp_config_playwright_renderer.go @@ -10,9 +10,7 @@ // Key responsibilities: // - Generating Playwright MCP server configuration // - Managing Docker container setup for Playwright -// - Handling allowed domains for browser navigation // - Processing custom Playwright arguments -// - Extracting and managing domain secrets from expressions // - Rendering configuration for different AI engines // // Container configuration: @@ -30,16 +28,9 @@ // runners. Without these flags, Playwright initialization fails with "EOF" error because // Chromium crashes during startup due to sandbox constraints. // -// Domain restrictions: -// For security, Playwright is restricted to specific allowed domains configured -// in the workflow frontmatter. These domains are passed via: -// - --allowed-hosts: Domains the browser can navigate to -// - --allowed-origins: Domains that can be used as origins -// -// Expression handling: -// When allowed_domains contains GitHub Actions expressions like ${{ secrets.DOMAIN }}, -// these are extracted and made available as environment variables. The actual -// secret values are resolved at runtime and passed to the Playwright container. +// Network access: +// Network egress for Playwright is controlled by the workflow firewall (network.allowed). +// Use the top-level network configuration to specify allowed domains. // // Engine compatibility: // The renderer supports multiple AI engines with engine-specific formatting: @@ -57,13 +48,13 @@ // tools: // playwright: // version: v1.41.0 -// allowed_domains: -// - github.com -// - api.github.com -// - ${{ secrets.CUSTOM_DOMAIN }} -// custom_args: +// args: // - --debug // - --timeout=30000 +// network: +// allowed: +// - github.com +// - api.github.com package workflow import ( @@ -86,14 +77,12 @@ func renderPlaywrightMCPConfig(yaml *strings.Builder, playwrightConfig *Playwrig // Per MCP Gateway Specification v1.0.0 section 3.2.1, stdio-based MCP servers MUST be containerized. // Uses MCP Gateway spec format: container, entrypointArgs, mounts, and args fields. func renderPlaywrightMCPConfigWithOptions(yaml *strings.Builder, playwrightConfig *PlaywrightToolConfig, isLast bool, includeCopilotFields bool, inlineArgs bool) { - args := generatePlaywrightDockerArgs(playwrightConfig) customArgs := getPlaywrightCustomArgs(playwrightConfig) // Extract all expressions from playwright arguments and replace them with env var references - expressions := extractExpressionsFromPlaywrightArgs(args.AllowedDomains, customArgs) - allowedDomains := replaceExpressionsInPlaywrightArgs(args.AllowedDomains, expressions) + expressions := extractExpressionsFromPlaywrightArgs(customArgs) - // Also replace expressions in custom args + // Replace expressions in custom args if len(customArgs) > 0 { customArgs = replaceExpressionsInPlaywrightArgs(customArgs, expressions) } @@ -140,15 +129,6 @@ func renderPlaywrightMCPConfigWithOptions(yaml *strings.Builder, playwrightConfi // Build entrypoint args for Playwright MCP server (goes after container image) entrypointArgs := []string{"--output-dir", "/tmp/gh-aw/mcp-logs/playwright"} - if len(allowedDomains) > 0 { - // Per Playwright MCP documentation: - // --allowed-hosts expects comma-separated list - // --allowed-origins expects semicolon-separated list - allowedHostsStr := strings.Join(allowedDomains, ",") - allowedOriginsStr := strings.Join(allowedDomains, ";") - entrypointArgs = append(entrypointArgs, "--allowed-hosts", allowedHostsStr) - entrypointArgs = append(entrypointArgs, "--allowed-origins", allowedOriginsStr) - } // Append custom args if present if len(customArgs) > 0 { entrypointArgs = append(entrypointArgs, customArgs...) diff --git a/pkg/workflow/mcp_config_shared_test.go b/pkg/workflow/mcp_config_shared_test.go index 97b2f3b0b7..da981f151a 100644 --- a/pkg/workflow/mcp_config_shared_test.go +++ b/pkg/workflow/mcp_config_shared_test.go @@ -156,9 +156,7 @@ func TestEngineMethodsDelegateToShared(t *testing.T) { IsLast: false, }) var yaml strings.Builder - playwrightTool := map[string]any{ - "allowed_domains": []any{"example.com"}, - } + playwrightTool := map[string]any{} renderer.RenderPlaywrightMCP(&yaml, playwrightTool) result := yaml.String() @@ -177,9 +175,7 @@ func TestEngineMethodsDelegateToShared(t *testing.T) { IsLast: false, }) var yaml strings.Builder - playwrightTool := map[string]any{ - "allowed_domains": []any{"example.com"}, - } + playwrightTool := map[string]any{} renderer.RenderPlaywrightMCP(&yaml, playwrightTool) result := yaml.String() @@ -206,9 +202,7 @@ func TestEngineMethodsDelegateToShared(t *testing.T) { IsLast: false, }) - playwrightTool := map[string]any{ - "allowed_domains": []any{"example.com", "test.com"}, - } + playwrightTool := map[string]any{} var claudeYAML strings.Builder claudeRenderer.RenderPlaywrightMCP(&claudeYAML, playwrightTool) diff --git a/pkg/workflow/mcp_config_validation.go b/pkg/workflow/mcp_config_validation.go index cc5733ae7d..ebc12566a3 100644 --- a/pkg/workflow/mcp_config_validation.go +++ b/pkg/workflow/mcp_config_validation.go @@ -124,30 +124,28 @@ func getRawMCPConfig(toolConfig map[string]any) (map[string]any, error) { // List of all known tool config fields (not just MCP) knownToolFields := map[string]bool{ - "type": true, - "url": true, - "command": true, - "container": true, - "env": true, - "headers": true, - "version": true, - "args": true, - "entrypoint": true, - "entrypointArgs": true, - "mounts": true, - "proxy-args": true, - "registry": true, - "allowed": true, - "mode": true, // for github tool - "github-token": true, // for github tool - "read-only": true, // for github tool - "toolsets": true, // for github tool - "id": true, // for cache-memory (array notation) - "key": true, // for cache-memory - "description": true, // for cache-memory - "retention-days": true, // for cache-memory - "allowed_domains": true, // for playwright tool - "allowed-domains": true, // for playwright tool (alternative notation) + "type": true, + "url": true, + "command": true, + "container": true, + "env": true, + "headers": true, + "version": true, + "args": true, + "entrypoint": true, + "entrypointArgs": true, + "mounts": true, + "proxy-args": true, + "registry": true, + "allowed": true, + "mode": true, // for github tool + "github-token": true, // for github tool + "read-only": true, // for github tool + "toolsets": true, // for github tool + "id": true, // for cache-memory (array notation) + "key": true, // for cache-memory + "description": true, // for cache-memory + "retention-days": true, // for cache-memory } // Check new format: direct fields in tool config diff --git a/pkg/workflow/mcp_environment.go b/pkg/workflow/mcp_environment.go index 7a5116bbd6..7e91c734ff 100644 --- a/pkg/workflow/mcp_environment.go +++ b/pkg/workflow/mcp_environment.go @@ -20,7 +20,7 @@ // - Safe Outputs: GH_AW_SAFE_OUTPUTS_*, GH_AW_ASSETS_* // - Safe Inputs: GH_AW_SAFE_INPUTS_PORT, GH_AW_SAFE_INPUTS_API_KEY // - Serena: GH_AW_SERENA_PORT (local mode only) -// - Playwright: Domain secrets from allowed_domains expressions +// - Playwright: Secrets from custom args expressions // - HTTP MCP: Custom secrets from headers and env sections // // Token precedence for GitHub MCP: @@ -153,13 +153,12 @@ func collectMCPEnvironmentVariables(tools map[string]any, mcpTools []string, wor } } if hasPlaywright { - // Extract all expressions from playwright arguments using ExpressionExtractor + // Extract all expressions from playwright custom args using ExpressionExtractor if playwrightTool, ok := tools["playwright"]; ok { playwrightConfig := parsePlaywrightTool(playwrightTool) - allowedDomains := generatePlaywrightAllowedDomains(playwrightConfig) customArgs := getPlaywrightCustomArgs(playwrightConfig) - playwrightAllowedDomainsSecrets := extractExpressionsFromPlaywrightArgs(allowedDomains, customArgs) - for envVarName, originalExpr := range playwrightAllowedDomainsSecrets { + playwrightArgSecrets := extractExpressionsFromPlaywrightArgs(customArgs) + for envVarName, originalExpr := range playwrightArgSecrets { envVars[envVarName] = originalExpr } } diff --git a/pkg/workflow/mcp_logs_upload_test.go b/pkg/workflow/mcp_logs_upload_test.go index 026f806eaf..0c40048590 100644 --- a/pkg/workflow/mcp_logs_upload_test.go +++ b/pkg/workflow/mcp_logs_upload_test.go @@ -21,7 +21,6 @@ on: workflow_dispatch: tools: playwright: - allowed_domains: ["example.com"] engine: claude --- diff --git a/pkg/workflow/mcp_playwright_config.go b/pkg/workflow/mcp_playwright_config.go index f653f6f470..f0c4599fe6 100644 --- a/pkg/workflow/mcp_playwright_config.go +++ b/pkg/workflow/mcp_playwright_config.go @@ -4,14 +4,12 @@ import ( "strings" "github.com/github/gh-aw/pkg/constants" - "github.com/github/gh-aw/pkg/parser" ) // PlaywrightDockerArgs represents the common Docker arguments for Playwright container type PlaywrightDockerArgs struct { ImageVersion string // Version for Docker image (mcr.microsoft.com/playwright:version) MCPPackageVersion string // Version for NPM package (@playwright/mcp@version) - AllowedDomains []string } func getPlaywrightDockerImageVersion(playwrightConfig *PlaywrightToolConfig) string { @@ -30,53 +28,24 @@ func getPlaywrightMCPPackageVersion(playwrightConfig *PlaywrightToolConfig) stri return string(constants.DefaultPlaywrightMCPVersion) } -// generatePlaywrightAllowedDomains extracts domain list from Playwright tool configuration with bundle resolution -// Uses the same domain bundle resolution as top-level network configuration, defaulting to localhost only -func generatePlaywrightAllowedDomains(playwrightConfig *PlaywrightToolConfig) []string { - // Default to localhost with all port variations (same as Copilot coding agent default) - allowedDomains := constants.DefaultAllowedDomains - - // Extract allowed_domains from Playwright tool configuration - if playwrightConfig != nil && len(playwrightConfig.AllowedDomains) > 0 { - // Create a mock NetworkPermissions structure to use the same domain resolution logic - playwrightNetwork := &NetworkPermissions{ - Allowed: playwrightConfig.AllowedDomains.ToStringSlice(), - } - - // Use the same domain bundle resolution as the top-level network configuration - resolvedDomains := GetAllowedDomains(playwrightNetwork) - - // Ensure localhost domains are always included - allowedDomains = parser.EnsureLocalhostDomains(resolvedDomains) - } - - return allowedDomains -} - // generatePlaywrightDockerArgs creates the common Docker arguments for Playwright MCP server func generatePlaywrightDockerArgs(playwrightConfig *PlaywrightToolConfig) PlaywrightDockerArgs { return PlaywrightDockerArgs{ ImageVersion: getPlaywrightDockerImageVersion(playwrightConfig), MCPPackageVersion: getPlaywrightMCPPackageVersion(playwrightConfig), - AllowedDomains: generatePlaywrightAllowedDomains(playwrightConfig), } } // extractExpressionsFromPlaywrightArgs extracts all GitHub Actions expressions from playwright arguments // Returns a map of environment variable names to their original expressions // Uses the same ExpressionExtractor as used for shell script security -func extractExpressionsFromPlaywrightArgs(allowedDomains []string, customArgs []string) map[string]string { - // Combine all arguments into a single string for extraction - var allArgs []string - allArgs = append(allArgs, allowedDomains...) - allArgs = append(allArgs, customArgs...) - - if len(allArgs) == 0 { +func extractExpressionsFromPlaywrightArgs(customArgs []string) map[string]string { + if len(customArgs) == 0 { return make(map[string]string) } // Join all arguments with a separator that won't appear in expressions - combined := strings.Join(allArgs, "\n") + combined := strings.Join(customArgs, "\n") // Use ExpressionExtractor to find all expressions extractor := NewExpressionExtractor() diff --git a/pkg/workflow/mcp_renderer.go b/pkg/workflow/mcp_renderer.go index 67eb9a3da5..194eb795be 100644 --- a/pkg/workflow/mcp_renderer.go +++ b/pkg/workflow/mcp_renderer.go @@ -215,7 +215,6 @@ func (r *MCPConfigRendererUnified) RenderPlaywrightMCP(yaml *strings.Builder, pl // Per MCP Gateway Specification v1.0.0 section 3.2.1, stdio-based MCP servers MUST be containerized. // Uses MCP Gateway spec format: container, entrypointArgs, mounts, and args fields. func (r *MCPConfigRendererUnified) renderPlaywrightTOML(yaml *strings.Builder, playwrightConfig *PlaywrightToolConfig) { - args := generatePlaywrightDockerArgs(playwrightConfig) customArgs := getPlaywrightCustomArgs(playwrightConfig) // Use official Playwright MCP Docker image (no version tag - only one image) @@ -242,14 +241,6 @@ func (r *MCPConfigRendererUnified) renderPlaywrightTOML(yaml *strings.Builder, p yaml.WriteString(" entrypointArgs = [\n") yaml.WriteString(" \"--output-dir\",\n") yaml.WriteString(" \"/tmp/gh-aw/mcp-logs/playwright\"") - if len(args.AllowedDomains) > 0 { - domainsStr := strings.Join(args.AllowedDomains, ";") - yaml.WriteString(",\n") - yaml.WriteString(" \"--allowed-hosts\",\n") - yaml.WriteString(" \"" + domainsStr + "\",\n") - yaml.WriteString(" \"--allowed-origins\",\n") - yaml.WriteString(" \"" + domainsStr + "\"") - } // Append custom args if present writeArgsToYAML(yaml, customArgs, " ") diff --git a/pkg/workflow/playwright_allowed_domains_secrets_test.go b/pkg/workflow/playwright_allowed_domains_secrets_test.go index c5b197a2fd..9a1e73483a 100644 --- a/pkg/workflow/playwright_allowed_domains_secrets_test.go +++ b/pkg/workflow/playwright_allowed_domains_secrets_test.go @@ -3,208 +3,42 @@ package workflow import ( - "os" "strings" "testing" ) -// TestPlaywrightAllowedDomainsSecretHandling tests that expressions in allowed_domains -// are properly extracted and replaced with environment variable references -func TestPlaywrightAllowedDomainsSecretHandling(t *testing.T) { - tests := []struct { - name string - workflow string - expectEnvVarPrefix string - expectMCPConfigValue string - expectRedaction bool - expectSecretName string - }{ - { - name: "Single secret in allowed_domains", - workflow: `--- -on: issues -engine: copilot -tools: - playwright: - allowed_domains: - - "${{ secrets.TEST_DOMAIN }}" ---- - -# Test workflow - -Test secret in allowed_domains. -`, - expectEnvVarPrefix: "GH_AW_SECRETS_", - expectMCPConfigValue: "__GH_AW_SECRETS_", - expectRedaction: true, - expectSecretName: "TEST_DOMAIN", - }, - { - name: "Multiple secrets in allowed_domains", - workflow: `--- -on: issues -engine: copilot -tools: - playwright: - allowed_domains: - - "${{ secrets.API_KEY }}" - - "example.com" - - "${{ secrets.ANOTHER_SECRET }}" ---- - -# Test workflow - -Test multiple secrets in allowed_domains. -`, - expectEnvVarPrefix: "GH_AW_SECRETS_", - expectMCPConfigValue: "__GH_AW_SECRETS_", - expectRedaction: true, - expectSecretName: "API_KEY", - }, - { - name: "No secrets in allowed_domains", - workflow: `--- -on: issues -engine: copilot -tools: - playwright: - allowed_domains: - - "example.com" - - "test.org" ---- - -# Test workflow - -Test no secrets in allowed_domains. -`, - expectEnvVarPrefix: "", - expectMCPConfigValue: "example.com", - expectRedaction: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Create temporary file - tmpFile, err := os.CreateTemp("", "test-playwright-secrets-*.md") - if err != nil { - t.Fatalf("Failed to create temp file: %v", err) - } - defer os.Remove(tmpFile.Name()) - - // Write content to file - if _, err := tmpFile.WriteString(tt.workflow); err != nil { - t.Fatalf("Failed to write to temp file: %v", err) - } - tmpFile.Close() - - // Create compiler and compile workflow - compiler := NewCompiler() - compiler.SetSkipValidation(true) - - // Parse the workflow file to get WorkflowData - workflowData, err := compiler.ParseWorkflowFile(tmpFile.Name()) - if err != nil { - t.Fatalf("Failed to parse workflow file: %v", err) - } - - // Generate YAML - yamlContent, err := compiler.generateYAML(workflowData, tmpFile.Name()) - if err != nil { - t.Fatalf("Failed to generate YAML: %v", err) - } - - // Check if environment variable with correct prefix exists in Start MCP Gateway step - if tt.expectRedaction { - if !strings.Contains(yamlContent, tt.expectEnvVarPrefix) { - t.Errorf("Expected environment variable with prefix %s not found in Start MCP Gateway step", tt.expectEnvVarPrefix) - } - } else { - if strings.Contains(yamlContent, tt.expectEnvVarPrefix) && tt.expectEnvVarPrefix != "" { - t.Errorf("Unexpected environment variable with prefix %s found when no secrets present", tt.expectEnvVarPrefix) - } - } - - // Check if MCP config uses environment variable reference - if tt.expectRedaction { - if !strings.Contains(yamlContent, tt.expectMCPConfigValue) { - t.Errorf("Expected MCP config to contain %s but it didn't", tt.expectMCPConfigValue) - } - - // Ensure the secret expression itself is NOT in the MCP config JSON - // (it should only be in env vars and redaction step) - mcpConfigStart := strings.Index(yamlContent, "cat > /home/runner/.copilot/mcp-config.json << EOF") - if mcpConfigStart != -1 { - mcpConfigEnd := strings.Index(yamlContent[mcpConfigStart:], "EOF\n") - if mcpConfigEnd != -1 { - mcpConfig := yamlContent[mcpConfigStart : mcpConfigStart+mcpConfigEnd] - if strings.Contains(mcpConfig, "${{ secrets.") { - t.Errorf("MCP config should not contain secret expressions, found secret in config") - } - } - } - } - - // Check if secret is in redaction list - if tt.expectRedaction && tt.expectSecretName != "" { - expectedRedactionEnv := "SECRET_" + tt.expectSecretName + ": ${{ secrets." + tt.expectSecretName + " }}" - if !strings.Contains(yamlContent, expectedRedactionEnv) { - t.Errorf("Expected secret %s to be in redaction step environment variables", tt.expectSecretName) - } - } - }) - } -} - // TestExtractExpressionsFromPlaywrightArgs tests the helper function func TestExtractExpressionsFromPlaywrightArgs(t *testing.T) { tests := []struct { name string - allowedDomains []string customArgs []string expectedExpressions int // Number of unique expressions expected }{ { - name: "Single expression in allowed_domains", - allowedDomains: []string{ - "${{ secrets.TEST_DOMAIN }}", - }, - customArgs: nil, + name: "Single expression in custom args", + customArgs: []string{"--arg", "${{ secrets.TEST_DOMAIN }}"}, expectedExpressions: 1, }, { - name: "Multiple expressions", - allowedDomains: []string{ - "${{ secrets.API_KEY }}", - "example.com", - "${{ secrets.ANOTHER_SECRET }}", - }, - customArgs: []string{"--arg", "${{ github.event.issue.number }}"}, - expectedExpressions: 3, - }, - { - name: "Expressions with whitespace", - allowedDomains: []string{ - "${{secrets.TEST_SECRET}}", - "${{ secrets.SPACED_SECRET }}", - }, - customArgs: nil, + name: "Multiple expressions", + customArgs: []string{"--arg", "${{ github.event.issue.number }}", "--secret", "${{ secrets.API_KEY }}"}, expectedExpressions: 2, }, { - name: "No expressions", - allowedDomains: []string{ - "example.com", - "test.org", - }, + name: "No expressions", customArgs: []string{"--static-arg"}, expectedExpressions: 0, }, + { + name: "Empty args", + customArgs: nil, + expectedExpressions: 0, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - expressions := extractExpressionsFromPlaywrightArgs(tt.allowedDomains, tt.customArgs) + expressions := extractExpressionsFromPlaywrightArgs(tt.customArgs) if len(expressions) != tt.expectedExpressions { t.Errorf("Expected %d expressions, got %d", tt.expectedExpressions, len(expressions)) diff --git a/pkg/workflow/playwright_mcp_integration_test.go b/pkg/workflow/playwright_mcp_integration_test.go index 11a3e37e29..85768956c0 100644 --- a/pkg/workflow/playwright_mcp_integration_test.go +++ b/pkg/workflow/playwright_mcp_integration_test.go @@ -4,18 +4,14 @@ package workflow import ( "os" - "os/exec" "path/filepath" "strings" "testing" "github.com/github/gh-aw/pkg/stringutil" - - "github.com/github/gh-aw/pkg/constants" ) -// TestPlaywrightMCPIntegration tests that compiled workflows generate correct Docker Playwright commands -// This test verifies that the official Playwright MCP Docker image is used with both --allowed-hosts and --allowed-origins flags +// TestPlaywrightMCPIntegration tests that compiled workflows generate correct Playwright MCP configuration func TestPlaywrightMCPIntegration(t *testing.T) { // Create a temporary directory for test files tmpDir, err := os.MkdirTemp("", "gh-aw-playwright-integration-*") @@ -27,32 +23,27 @@ func TestPlaywrightMCPIntegration(t *testing.T) { tests := []struct { name string workflowContent string - expectedFlags []string - expectedDomains []string shouldContainPackage bool + notExpectedFlags []string }{ { - name: "Codex engine with playwright and custom domains", + name: "Codex engine with playwright", workflowContent: `--- on: push engine: codex tools: playwright: - allowed_domains: - - "example.com" - - "test.com" --- # Test Workflow -Test playwright with custom domains. +Test playwright with codex engine. `, - expectedFlags: []string{"--allowed-hosts", "--allowed-origins"}, - expectedDomains: []string{"example.com", "test.com", "localhost", "127.0.0.1"}, shouldContainPackage: true, + notExpectedFlags: []string{"--allowed-hosts", "--allowed-origins"}, }, { - name: "Claude engine with playwright default domains", + name: "Claude engine with playwright", workflowContent: `--- on: push engine: claude @@ -62,11 +53,10 @@ tools: # Test Workflow -Test playwright with default domains only. +Test playwright with default settings. `, - expectedFlags: []string{"--allowed-hosts", "--allowed-origins"}, - expectedDomains: []string{"localhost", "127.0.0.1"}, shouldContainPackage: true, + notExpectedFlags: []string{"--allowed-hosts", "--allowed-origins"}, }, { name: "Copilot engine with playwright", @@ -75,17 +65,14 @@ on: push engine: copilot tools: playwright: - allowed_domains: - - "github.com" --- # Test Workflow Test playwright with copilot engine. `, - expectedFlags: []string{"--allowed-hosts", "--allowed-origins"}, - expectedDomains: []string{"github.com", "localhost", "127.0.0.1"}, shouldContainPackage: true, + notExpectedFlags: []string{"--allowed-hosts", "--allowed-origins"}, }, } @@ -120,50 +107,12 @@ Test playwright with copilot engine. } } - // Verify all expected flags are used - for _, flag := range tt.expectedFlags { - if !strings.Contains(lockStr, flag) { - t.Errorf("Expected lock file to contain flag %s\nActual content:\n%s", flag, lockStr) - } - } - - // Verify expected domains are present - for _, domain := range tt.expectedDomains { - if !strings.Contains(lockStr, domain) { - t.Errorf("Expected lock file to contain domain %s", domain) + // Verify egress flags are NOT present (controlled by firewall, not playwright flags) + for _, flag := range tt.notExpectedFlags { + if strings.Contains(lockStr, flag) { + t.Errorf("Lock file should not contain flag %s (egress is controlled by firewall)", flag) } } }) } } - -// TestPlaywrightNPXCommandWorks verifies that the generated npx command actually works -// This test requires npx to be available and will be skipped if it's not -func TestPlaywrightNPXCommandWorks(t *testing.T) { - // Check if npx is available - if _, err := exec.LookPath("npx"); err != nil { - t.Skip("npx not found, skipping live integration test") - } - - // Test that the npx command with --allowed-hosts flag works - cmd := exec.Command("npx", "@playwright/mcp@"+string(constants.DefaultPlaywrightMCPVersion), "--help") - output, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("Failed to run npx playwright help: %v\nOutput: %s", err, output) - } - - outputStr := string(output) - - // Verify that --allowed-hosts and --allowed-origins are in the help output - if !strings.Contains(outputStr, "--allowed-hosts") { - t.Errorf("Expected npx playwright help to mention --allowed-hosts flag\nActual output:\n%s", outputStr) - } - if !strings.Contains(outputStr, "--allowed-origins") { - t.Errorf("Expected npx playwright help to mention --allowed-origins flag\nActual output:\n%s", outputStr) - } - - // Note: --allowed-origins was added in v0.0.48 for browser request filtering - // --allowed-hosts controls which hosts the MCP server serves from (CORS) - // --allowed-origins controls which origins the Playwright browser can navigate to - // Both flags are now used together for complete network control -} diff --git a/pkg/workflow/sandbox_validation.go b/pkg/workflow/sandbox_validation.go index d08b92a5e8..360ce0b1d1 100644 --- a/pkg/workflow/sandbox_validation.go +++ b/pkg/workflow/sandbox_validation.go @@ -143,7 +143,7 @@ func validateSandboxConfig(workflowData *WorkflowData) error { "sandbox", "enabled without MCP servers", "agent sandbox requires MCP servers to be configured", - "Add MCP tools to your workflow:\n\ntools:\n github:\n mode: remote\n playwright:\n allowed_domains: [\"example.com\"]\n\nOr disable the agent sandbox:\nsandbox:\n agent: false", + "Add MCP tools to your workflow:\n\ntools:\n github:\n mode: remote\n playwright: null\n\nOr disable the agent sandbox:\nsandbox:\n agent: false", ) } if hasExplicitSandboxConfig { diff --git a/pkg/workflow/shared_workflow_test.go b/pkg/workflow/shared_workflow_test.go index 27b63e4090..333e2e9b82 100644 --- a/pkg/workflow/shared_workflow_test.go +++ b/pkg/workflow/shared_workflow_test.go @@ -24,8 +24,6 @@ description: "Shared configuration without on field" tools: playwright: version: "v1.41.0" - allowed_domains: - - "example.com" network: allowed: - playwright diff --git a/pkg/workflow/strict_mode_test.go b/pkg/workflow/strict_mode_test.go index ef2c8351e7..c15db0acc3 100644 --- a/pkg/workflow/strict_mode_test.go +++ b/pkg/workflow/strict_mode_test.go @@ -168,7 +168,6 @@ network: tools: github: false playwright: - allowed_domains: ["example.com"] --- # Test Workflow`, diff --git a/pkg/workflow/testdata/wasm_golden/TestWasmGolden_CompileFixtures/smoke-copilot.golden b/pkg/workflow/testdata/wasm_golden/TestWasmGolden_CompileFixtures/smoke-copilot.golden index 48987c45ac..246b4a8665 100644 --- a/pkg/workflow/testdata/wasm_golden/TestWasmGolden_CompileFixtures/smoke-copilot.golden +++ b/pkg/workflow/testdata/wasm_golden/TestWasmGolden_CompileFixtures/smoke-copilot.golden @@ -493,7 +493,7 @@ jobs: "type": "stdio", "container": "mcr.microsoft.com/playwright/mcp", "args": ["--init", "--network", "host", "--security-opt", "seccomp=unconfined", "--ipc=host"], - "entrypointArgs": ["--output-dir", "/tmp/gh-aw/mcp-logs/playwright", "--allowed-hosts", "localhost,localhost:*,127.0.0.1,127.0.0.1:*,github.com", "--allowed-origins", "localhost;localhost:*;127.0.0.1;127.0.0.1:*;github.com"], + "entrypointArgs": ["--output-dir", "/tmp/gh-aw/mcp-logs/playwright"], "mounts": ["/tmp/gh-aw/mcp-logs:/tmp/gh-aw/mcp-logs:rw"] }, "serena": { diff --git a/pkg/workflow/testdata/wasm_golden/fixtures/smoke-copilot.md b/pkg/workflow/testdata/wasm_golden/fixtures/smoke-copilot.md index e780b091e6..7f5c7f2445 100644 --- a/pkg/workflow/testdata/wasm_golden/fixtures/smoke-copilot.md +++ b/pkg/workflow/testdata/wasm_golden/fixtures/smoke-copilot.md @@ -34,8 +34,6 @@ tools: - "*" github: playwright: - allowed_domains: - - github.com serena: languages: go: {} diff --git a/pkg/workflow/tools_parser.go b/pkg/workflow/tools_parser.go index f5fea96142..0cc33be62e 100644 --- a/pkg/workflow/tools_parser.go +++ b/pkg/workflow/tools_parser.go @@ -299,25 +299,6 @@ func parsePlaywrightTool(val any) *PlaywrightToolConfig { config.Version = fmt.Sprintf("%g", versionNum) } - // Handle allowed_domains - can be string or array - if allowedDomains, ok := configMap["allowed_domains"]; ok { - if str, ok := allowedDomains.(string); ok { - config.AllowedDomains = PlaywrightAllowedDomains{PlaywrightDomain(str)} - } else if arr, ok := allowedDomains.([]any); ok { - config.AllowedDomains = make(PlaywrightAllowedDomains, 0, len(arr)) - for _, item := range arr { - if str, ok := item.(string); ok { - config.AllowedDomains = append(config.AllowedDomains, PlaywrightDomain(str)) - } - } - } else if arr, ok := allowedDomains.([]string); ok { - config.AllowedDomains = make(PlaywrightAllowedDomains, 0, len(arr)) - for _, str := range arr { - config.AllowedDomains = append(config.AllowedDomains, PlaywrightDomain(str)) - } - } - } - // Handle args field - can be []any or []string if argsValue, ok := configMap["args"]; ok { if arr, ok := argsValue.([]any); ok { diff --git a/pkg/workflow/tools_types.go b/pkg/workflow/tools_types.go index d936c8696e..db86020a6b 100644 --- a/pkg/workflow/tools_types.go +++ b/pkg/workflow/tools_types.go @@ -271,26 +271,10 @@ type GitHubToolConfig struct { App *GitHubAppConfig `yaml:"app,omitempty"` // GitHub App configuration for token minting } -// PlaywrightDomain represents a domain name allowed for Playwright browser automation -type PlaywrightDomain string - -// PlaywrightAllowedDomains is a slice of allowed domain names for Playwright -type PlaywrightAllowedDomains []PlaywrightDomain - -// ToStringSlice converts PlaywrightAllowedDomains to []string -func (p PlaywrightAllowedDomains) ToStringSlice() []string { - result := make([]string, len(p)) - for i, domain := range p { - result[i] = string(domain) - } - return result -} - // PlaywrightToolConfig represents the configuration for the Playwright tool type PlaywrightToolConfig struct { - Version string `yaml:"version,omitempty"` - AllowedDomains PlaywrightAllowedDomains `yaml:"allowed_domains,omitempty"` - Args []string `yaml:"args,omitempty"` + Version string `yaml:"version,omitempty"` + Args []string `yaml:"args,omitempty"` } // SerenaToolConfig represents the configuration for the Serena MCP tool diff --git a/pkg/workflow/tools_types_test.go b/pkg/workflow/tools_types_test.go index f8b00ddc71..7f452ced42 100644 --- a/pkg/workflow/tools_types_test.go +++ b/pkg/workflow/tools_types_test.go @@ -270,9 +270,8 @@ func TestPlaywrightConfigParsing(t *testing.T) { t.Run("parses playwright config map", func(t *testing.T) { toolsMap := map[string]any{ "playwright": map[string]any{ - "version": "v1.41.0", - "allowed_domains": []any{"github.com", "example.com"}, - "args": []any{"--headless"}, + "version": "v1.41.0", + "args": []any{"--headless"}, }, } @@ -287,10 +286,6 @@ func TestPlaywrightConfigParsing(t *testing.T) { t.Errorf("expected version 'v1.41.0', got %q", config.Version) } - if len(config.AllowedDomains) != 2 { - t.Errorf("expected 2 allowed domains, got %d", len(config.AllowedDomains)) - } - if len(config.Args) != 1 { t.Errorf("expected 1 arg, got %d", len(config.Args)) } diff --git a/pkg/workflow/version_field_test.go b/pkg/workflow/version_field_test.go index 740fd190e4..f6cca57b8a 100644 --- a/pkg/workflow/version_field_test.go +++ b/pkg/workflow/version_field_test.go @@ -64,8 +64,7 @@ func TestVersionField(t *testing.T) { t.Run("Playwright version field extraction", func(t *testing.T) { // Test "version" field playwrightTool := map[string]any{ - "allowed_domains": []any{"example.com"}, - "version": "v1.41.0", + "version": "v1.41.0", } playwrightConfig := parsePlaywrightTool(playwrightTool) result := getPlaywrightDockerImageVersion(playwrightConfig) @@ -74,9 +73,7 @@ func TestVersionField(t *testing.T) { } // Test default value when version field is not present - playwrightToolDefault := map[string]any{ - "allowed_domains": []any{"example.com"}, - } + playwrightToolDefault := map[string]any{} playwrightConfigDefault := parsePlaywrightTool(playwrightToolDefault) result = getPlaywrightDockerImageVersion(playwrightConfigDefault) if result != string(constants.DefaultPlaywrightBrowserVersion) { @@ -85,8 +82,7 @@ func TestVersionField(t *testing.T) { // Test integer version playwrightToolInt := map[string]any{ - "allowed_domains": []any{"example.com"}, - "version": 20, + "version": 20, } playwrightConfigInt := parsePlaywrightTool(playwrightToolInt) result = getPlaywrightDockerImageVersion(playwrightConfigInt) @@ -96,8 +92,7 @@ func TestVersionField(t *testing.T) { // Test float version playwrightToolFloat := map[string]any{ - "allowed_domains": []any{"example.com"}, - "version": 1.41, + "version": 1.41, } playwrightConfigFloat := parsePlaywrightTool(playwrightToolFloat) result = getPlaywrightDockerImageVersion(playwrightConfigFloat) @@ -107,8 +102,7 @@ func TestVersionField(t *testing.T) { // Test int64 version playwrightToolInt64 := map[string]any{ - "allowed_domains": []any{"example.com"}, - "version": int64(142), + "version": int64(142), } playwrightConfigInt64 := parsePlaywrightTool(playwrightToolInt64) result = getPlaywrightDockerImageVersion(playwrightConfigInt64) @@ -153,8 +147,7 @@ func TestVersionField(t *testing.T) { frontmatterPlaywright := map[string]any{ "tools": map[string]any{ "playwright": map[string]any{ - "allowed_domains": []any{"example.com"}, - "version": "v1.41.0", + "version": "v1.41.0", }, }, } @@ -183,8 +176,7 @@ func TestVersionField(t *testing.T) { frontmatterPlaywrightInt := map[string]any{ "tools": map[string]any{ "playwright": map[string]any{ - "allowed_domains": []any{"example.com"}, - "version": 20, + "version": 20, }, }, } @@ -213,8 +205,7 @@ func TestVersionField(t *testing.T) { frontmatterPlaywrightFloat := map[string]any{ "tools": map[string]any{ "playwright": map[string]any{ - "allowed_domains": []any{"example.com"}, - "version": 1.41, + "version": 1.41, }, }, }