diff --git a/actions/setup/js/firewall_blocked_domains.cjs b/actions/setup/js/firewall_blocked_domains.cjs index 8f5210b9db0..de4ef1e17b7 100644 --- a/actions/setup/js/firewall_blocked_domains.cjs +++ b/actions/setup/js/firewall_blocked_domains.cjs @@ -14,6 +14,18 @@ const { sanitizeDomainName } = require("./sanitize_content_core.cjs"); const { renderTemplateFromFile, getPromptPath } = require("./messages_core.cjs"); const { renderMarkdownTemplate } = require("./render_template.cjs"); +// Internal AWF sidecar container hostnames added to network.topologyAttach by +// gh-aw itself (e.g. the MCP Gateway and the CLI proxy). These are +// framework-managed, not user-controllable external domains, and must never +// surface in the "blocked domains" warning shown on issues/PRs. +const AWF_INTERNAL_SIDECAR_HOSTS = ["awmg-mcpg", "awmg-cli-proxy"]; + +// Pre-compute sanitized forms at module load time. +// sanitizeDomainName strips non-alphanumeric characters (including hyphens), +// which is exactly how these container names appear after log sanitization +// (e.g. "awmg-mcpg" → "awmgmcpg", "awmg-cli-proxy" → "awmgcliproxy"). +const AWF_INTERNAL_SIDECAR_HOSTS_SANITIZED = new Set(AWF_INTERNAL_SIDECAR_HOSTS.map(h => sanitizeDomainName(h))); + /** * Parses a single firewall log line * Format: timestamp client_ip:port domain dest_ip:port proto method status decision url user_agent @@ -173,7 +185,7 @@ function getBlockedDomains(logsDir) { domainField = entry.destIpPort; } const sanitizedDomain = extractAndSanitizeDomain(domainField); - if (sanitizedDomain && sanitizedDomain !== "-") { + if (sanitizedDomain && sanitizedDomain !== "-" && !AWF_INTERNAL_SIDECAR_HOSTS_SANITIZED.has(sanitizedDomain)) { blockedDomainsSet.add(sanitizedDomain); } } diff --git a/actions/setup/js/firewall_blocked_domains.test.cjs b/actions/setup/js/firewall_blocked_domains.test.cjs index 494ee4305ce..d7fa446d642 100644 --- a/actions/setup/js/firewall_blocked_domains.test.cjs +++ b/actions/setup/js/firewall_blocked_domains.test.cjs @@ -303,6 +303,70 @@ describe("firewall_blocked_domains.cjs", () => { expect(result).toEqual(["blocked.example.com"]); }); + + it("should filter out internal AWF sidecar hostname awmg-mcpg", () => { + const logsDir = path.join(testDir, "logs-sidecar-mcpg"); + fs.mkdirSync(logsDir, { recursive: true }); + + // The MCP gateway sidecar (awmg-mcpg) appears in firewall logs as a + // blocked domain because it is in topologyAttach but not in allowDomains. + // It should be suppressed from the blocked-domains warning. + const logContent = [ + '1761332530.474 172.30.0.20:35288 awmg-mcpg:8080 10.0.0.1:8080 1.1 CONNECT 403 NONE_NONE:HIER_NONE awmg-mcpg:8080 "-"', + '1761332530.475 172.30.0.20:35289 blocked.example.com:443 140.82.112.22:443 1.1 CONNECT 403 NONE_NONE:HIER_NONE blocked.example.com:443 "-"', + '1761332530.476 172.30.0.20:35290 api.github.com:443 140.82.112.22:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.github.com:443 "-"', + ].join("\n"); + + fs.writeFileSync(path.join(logsDir, "access.log"), logContent); + + const result = getBlockedDomains(logsDir); + + // Real blocked domain should appear + expect(result).toContain("blocked.example.com"); + // Internal sidecar awmg-mcpg (sanitized: awmgmcpg) must be suppressed + expect(result).not.toContain("awmgmcpg"); + expect(result).not.toContain("awmg-mcpg"); + // Allowed domain must not appear + expect(result).not.toContain("api.github.com"); + }); + + it("should filter out internal AWF sidecar hostname awmg-cli-proxy", () => { + const logsDir = path.join(testDir, "logs-sidecar-cli-proxy"); + fs.mkdirSync(logsDir, { recursive: true }); + + const logContent = [ + '1761332530.474 172.30.0.20:35288 awmg-cli-proxy:3128 10.0.0.2:3128 1.1 CONNECT 403 NONE_NONE:HIER_NONE awmg-cli-proxy:3128 "-"', + '1761332530.475 172.30.0.20:35289 blocked.example.com:443 140.82.112.22:443 1.1 CONNECT 403 NONE_NONE:HIER_NONE blocked.example.com:443 "-"', + ].join("\n"); + + fs.writeFileSync(path.join(logsDir, "access.log"), logContent); + + const result = getBlockedDomains(logsDir); + + // Real blocked domain should appear + expect(result).toContain("blocked.example.com"); + // Internal sidecar awmg-cli-proxy (sanitized: awmgcliproxy) must be suppressed + expect(result).not.toContain("awmgcliproxy"); + expect(result).not.toContain("awmg-cli-proxy"); + }); + + it("should return empty array when only internal sidecar domains were blocked", () => { + const logsDir = path.join(testDir, "logs-sidecar-only"); + fs.mkdirSync(logsDir, { recursive: true }); + + const logContent = [ + '1761332530.474 172.30.0.20:35288 awmg-mcpg:8080 10.0.0.1:8080 1.1 CONNECT 403 NONE_NONE:HIER_NONE awmg-mcpg:8080 "-"', + '1761332530.475 172.30.0.20:35289 awmg-cli-proxy:3128 10.0.0.2:3128 1.1 CONNECT 403 NONE_NONE:HIER_NONE awmg-cli-proxy:3128 "-"', + '1761332530.476 172.30.0.20:35290 api.github.com:443 140.82.112.22:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.github.com:443 "-"', + ].join("\n"); + + fs.writeFileSync(path.join(logsDir, "access.log"), logContent); + + const result = getBlockedDomains(logsDir); + + // All sidecar entries suppressed, no real blocked domains → empty result + expect(result).toEqual([]); + }); }); describe("generateBlockedDomainsSection", () => { diff --git a/actions/setup/js/parse_firewall_logs.cjs b/actions/setup/js/parse_firewall_logs.cjs index daa9454afe6..4bafffd920f 100644 --- a/actions/setup/js/parse_firewall_logs.cjs +++ b/actions/setup/js/parse_firewall_logs.cjs @@ -7,6 +7,24 @@ const { sanitizeWorkflowName } = require("./sanitize_workflow_name.cjs"); const { ERR_PARSE } = require("./error_codes.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); +// Internal AWF sidecar container hostnames added to network.topologyAttach by +// gh-aw itself. These are framework-managed and should be excluded from blocked +// domain reporting in step summaries so they do not appear as actionable items. +const AWF_INTERNAL_SIDECAR_HOSTS = new Set(["awmg-mcpg", "awmg-cli-proxy"]); + +/** + * Returns true when domainKey refers to a framework-internal sidecar container. + * domainKey may be "hostname:port" or bare "hostname". + * @param {string} domainKey + * @returns {boolean} + */ +function isInternalSidecarHost(domainKey) { + if (!domainKey || domainKey === "-") return false; + const lastColon = domainKey.lastIndexOf(":"); + const host = lastColon > 0 ? domainKey.substring(0, lastColon) : domainKey; + return AWF_INTERNAL_SIDECAR_HOSTS.has(host); +} + /** * Parses firewall logs and creates a step summary * Firewall log format: timestamp client_ip:port domain dest_ip:port proto method status decision url user_agent @@ -190,19 +208,28 @@ function analyzeFirewallLogLines(lines) { allowedRequests++; allowedDomains.add(domainKey); } else { - blockedRequests++; - blockedDomains.add(domainKey); + // Skip internal sidecar hostnames (awmg-mcpg, awmg-cli-proxy) from the + // blocked domain set. These are framework-managed topology-attach containers + // and are not user-actionable external blocked domains. + if (!isInternalSidecarHost(domainKey)) { + blockedRequests++; + blockedDomains.add(domainKey); + } } - // Track request count per domain - if (!requestsByDomain.has(domainKey)) { - requestsByDomain.set(domainKey, { allowed: 0, blocked: 0 }); - } - const domainStats = requestsByDomain.get(domainKey); - if (isAllowed) { - domainStats.allowed++; - } else { - domainStats.blocked++; + // Track request count per domain. + // Skip internal sidecar hostnames for blocked entries — they are already excluded from + // blockedRequests/blockedDomains above and must not appear in the summary domain table. + if (isAllowed || !isInternalSidecarHost(domainKey)) { + if (!requestsByDomain.has(domainKey)) { + requestsByDomain.set(domainKey, { allowed: 0, blocked: 0 }); + } + const domainStats = requestsByDomain.get(domainKey); + if (isAllowed) { + domainStats.allowed++; + } else { + domainStats.blocked++; + } } } @@ -266,6 +293,7 @@ if (typeof module !== "undefined" && module.exports) { isRequestAllowed, analyzeFirewallLogLines, generateFirewallSummary, + isInternalSidecarHost, main, }; } diff --git a/actions/setup/js/parse_firewall_logs.test.cjs b/actions/setup/js/parse_firewall_logs.test.cjs index 2f71b971ca3..e54fd804b5c 100644 --- a/actions/setup/js/parse_firewall_logs.test.cjs +++ b/actions/setup/js/parse_firewall_logs.test.cjs @@ -4,7 +4,7 @@ import path from "path"; const mockCore = { info: vi.fn(), setFailed: vi.fn(), summary: { addRaw: vi.fn().mockReturnThis(), write: vi.fn().mockResolvedValue() } }; ((global.core = mockCore), describe("parse_firewall_logs.cjs", () => { - let parseFirewallLogLine, isRequestAllowed, analyzeFirewallLogLines, generateFirewallSummary; + let parseFirewallLogLine, isRequestAllowed, analyzeFirewallLogLines, generateFirewallSummary, isInternalSidecarHost; (beforeEach(() => { vi.clearAllMocks(); const scriptPath = path.join(process.cwd(), "parse_firewall_logs.cjs"), @@ -13,13 +13,14 @@ const mockCore = { info: vi.fn(), setFailed: vi.fn(), summary: { addRaw: vi.fn() .replace(/if \(typeof module === "undefined".*?\) \{[\s\S]*?main\(\);[\s\S]*?\}/g, "// main() execution disabled for testing") .replace( "// Export for testing", - "global.testParseFirewallLogLine = parseFirewallLogLine;\n global.testIsRequestAllowed = isRequestAllowed;\n global.testAnalyzeFirewallLogLines = analyzeFirewallLogLines;\n global.testGenerateFirewallSummary = generateFirewallSummary;\n // Export for testing" + "global.testParseFirewallLogLine = parseFirewallLogLine;\n global.testIsRequestAllowed = isRequestAllowed;\n global.testAnalyzeFirewallLogLines = analyzeFirewallLogLines;\n global.testGenerateFirewallSummary = generateFirewallSummary;\n global.testIsInternalSidecarHost = isInternalSidecarHost;\n // Export for testing" ); (eval(scriptForTesting), (parseFirewallLogLine = global.testParseFirewallLogLine), (isRequestAllowed = global.testIsRequestAllowed), (analyzeFirewallLogLines = global.testAnalyzeFirewallLogLines), - (generateFirewallSummary = global.testGenerateFirewallSummary)); + (generateFirewallSummary = global.testGenerateFirewallSummary), + (isInternalSidecarHost = global.testIsInternalSidecarHost)); }), describe("parseFirewallLogLine", () => { (test("should parse valid firewall log line", () => { @@ -243,5 +244,59 @@ const mockCore = { info: vi.fn(), setFailed: vi.fn(), summary: { addRaw: vi.fn() expect(summary).toContain("| api.github.com:443 | 10 | 0 |"), expect(summary).not.toContain("error:")); })); + }), + describe("isInternalSidecarHost", () => { + (test("should identify awmg-mcpg with port as internal sidecar", () => { + expect(isInternalSidecarHost("awmg-mcpg:8080")).toBe(true); + }), + test("should identify awmg-mcpg without port as internal sidecar", () => { + expect(isInternalSidecarHost("awmg-mcpg")).toBe(true); + }), + test("should identify awmg-cli-proxy with port as internal sidecar", () => { + expect(isInternalSidecarHost("awmg-cli-proxy:3128")).toBe(true); + }), + test("should not identify external domain as internal sidecar", () => { + expect(isInternalSidecarHost("api.github.com:443")).toBe(false); + }), + test("should not identify placeholder as internal sidecar", () => { + (expect(isInternalSidecarHost("-")).toBe(false), expect(isInternalSidecarHost("")).toBe(false), expect(isInternalSidecarHost(null)).toBe(false)); + })); + }), + describe("analyzeFirewallLogLines - internal sidecar filtering", () => { + (test("should not count awmg-mcpg blocked entries in blockedRequests", () => { + const lines = [ + '1761332530.474 172.30.0.20:35288 awmg-mcpg:8080 10.0.0.1:8080 1.1 CONNECT 403 NONE_NONE:HIER_NONE awmg-mcpg:8080 "-"', + '1761332530.475 172.30.0.20:35289 blocked.example.com:443 140.82.112.22:443 1.1 CONNECT 403 NONE_NONE:HIER_NONE blocked.example.com:443 "-"', + '1761332530.476 172.30.0.20:35290 api.github.com:443 140.82.112.22:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.github.com:443 "-"', + ]; + const result = analyzeFirewallLogLines(lines); + (expect(result.blockedRequests).toBe(1), + expect(result.blockedDomains.has("awmg-mcpg:8080")).toBe(false), + expect(result.blockedDomains.has("blocked.example.com:443")).toBe(true), + expect(result.requestsByDomain.has("awmg-mcpg:8080")).toBe(false)); + }), + test("should not count awmg-cli-proxy blocked entries in blockedRequests", () => { + const lines = [ + '1761332530.474 172.30.0.20:35288 awmg-cli-proxy:3128 10.0.0.2:3128 1.1 CONNECT 403 NONE_NONE:HIER_NONE awmg-cli-proxy:3128 "-"', + '1761332530.475 172.30.0.20:35289 blocked.example.com:443 1.2.3.4:443 1.1 CONNECT 403 NONE_NONE:HIER_NONE blocked.example.com:443 "-"', + ]; + const result = analyzeFirewallLogLines(lines); + (expect(result.blockedRequests).toBe(1), + expect(result.blockedDomains.has("awmg-cli-proxy:3128")).toBe(false), + expect(result.blockedDomains.has("blocked.example.com:443")).toBe(true), + expect(result.requestsByDomain.has("awmg-cli-proxy:3128")).toBe(false)); + }), + test("should report zero blocked requests when only sidecar entries were blocked", () => { + const lines = [ + '1761332530.474 172.30.0.20:35288 awmg-mcpg:8080 10.0.0.1:8080 1.1 CONNECT 403 NONE_NONE:HIER_NONE awmg-mcpg:8080 "-"', + '1761332530.475 172.30.0.20:35289 api.github.com:443 140.82.112.22:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.github.com:443 "-"', + ]; + const result = analyzeFirewallLogLines(lines); + (expect(result.totalRequests).toBe(2), + expect(result.blockedRequests).toBe(0), + expect(result.allowedRequests).toBe(1), + expect(result.blockedDomains.size).toBe(0), + expect(result.requestsByDomain.has("awmg-mcpg:8080")).toBe(false)); + })); })); })); diff --git a/actions/setup/js/start_mcp_gateway.cjs b/actions/setup/js/start_mcp_gateway.cjs index a3f15ed7e34..1b522a74dc8 100644 --- a/actions/setup/js/start_mcp_gateway.cjs +++ b/actions/setup/js/start_mcp_gateway.cjs @@ -101,6 +101,19 @@ function getJSONParseErrorContext(jsonText, parseErrorMessage) { return { line, column, lineText, key }; } +/** + * Repairs a known double-encoding regression where sink-visibility can be rendered as: + * "sink-visibility": ""public"" + * instead of: + * "sink-visibility": "public" + * + * @param {string} jsonText + * @returns {string} + */ +function normalizeSinkVisibilityEncoding(jsonText) { + return jsonText.replace(/("sink-visibility"\s*:\s*)""(public|private|internal)""/g, '$1"$2"'); +} + /** * Normalizes GH_AW_OTLP_IF_MISSING to a supported mode. * @param {string | undefined} value @@ -421,6 +434,11 @@ async function main() { } catch (err) { throw new Error(`Failed to read MCP configuration from stdin: ${String(err)}`, { cause: err }); } + const normalizedConfig = normalizeSinkVisibilityEncoding(mcpConfig); + if (normalizedConfig !== mcpConfig) { + core.warning("Detected double-encoded sink-visibility value in MCP config; applying compatibility normalization."); + mcpConfig = normalizedConfig; + } printTiming(configReadStart, "Configuration read from stdin"); core.info(""); @@ -1042,5 +1060,6 @@ module.exports = { hasNonEmptyOTLPHeaders, isOTLPIfMissingIgnore, getJSONParseErrorContext, + normalizeSinkVisibilityEncoding, resolveCopilotConfigPaths, }; diff --git a/actions/setup/js/start_mcp_gateway.test.cjs b/actions/setup/js/start_mcp_gateway.test.cjs index 6016ee1f29a..482b063a35f 100644 --- a/actions/setup/js/start_mcp_gateway.test.cjs +++ b/actions/setup/js/start_mcp_gateway.test.cjs @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { applyOTLPIgnoreIfMissing, detectEngineType, getJSONParseErrorContext, getOTLPIfMissingMode, hasNonEmptyOTLPHeaders, resolveCopilotConfigPaths } from "./start_mcp_gateway.cjs"; +import { applyOTLPIgnoreIfMissing, detectEngineType, getJSONParseErrorContext, getOTLPIfMissingMode, hasNonEmptyOTLPHeaders, normalizeSinkVisibilityEncoding, resolveCopilotConfigPaths } from "./start_mcp_gateway.cjs"; describe("start_mcp_gateway OTLP if-missing helpers", () => { let originalWarning; @@ -221,3 +221,27 @@ describe("start_mcp_gateway getJSONParseErrorContext", () => { expect(context?.lineText).toContain(`"GITHUB_HOST"`); }); }); + +describe("start_mcp_gateway normalizeSinkVisibilityEncoding", () => { + it("normalizes double-encoded sink visibility values", () => { + const invalidConfig = `{ + "guard-policies": { + "write-sink": { + "sink-visibility": ""public"" + } + } +}`; + expect(normalizeSinkVisibilityEncoding(invalidConfig)).toContain(`"sink-visibility": "public"`); + }); + + it("leaves correctly encoded sink visibility values unchanged", () => { + const validConfig = `{ + "guard-policies": { + "write-sink": { + "sink-visibility": "public" + } + } +}`; + expect(normalizeSinkVisibilityEncoding(validConfig)).toBe(validConfig); + }); +});