fix: suppress internal AWF sidecar hostnames from blocked-domain warnings #48591
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 { | ||
| // 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)) { | ||
|
Contributor
There was a problem hiding this comment. [/diagnosing-bugs] Sidecar requests increment 💡 Suggested fixEither also exclude sidecar requests from // Option A: exclude sidecar entries entirely from request counting
if (isInternalSidecarHost(domainKey)) {
continue; // skip totalRequests++ too
}
totalRequests++;@copilot please address this. |
||
| blockedRequests++; | ||
| blockedDomains.add(domainKey); | ||
| } | ||
|
Comment on lines
+214
to
+217
|
||
| } | ||
|
|
||
| // 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, | ||
| }; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, 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 global.testIsInternalSidecarHost = isInternalSidecarHost;\n // Export for testing" | ||
| ); | ||
| (eval(scriptForTesting), | ||
| (parseFirewallLogLine = global.testParseFirewallLogLine), | ||
| (isRequestAllowed = global.testIsRequestAllowed), | ||
| (analyzeFirewallLogLines = global.testAnalyzeFirewallLogLines), | ||
| (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)); | ||
| })); | ||
| })); | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The 💡 Suggested assertion// In the "should report zero blocked requests" test:
expect(result.totalRequests).toBe(2); // already present
expect(result.allowedRequests + result.blockedRequests).toBe(result.totalRequests); // this will FAIL todayAdding this assertion turns the coverage gap into a red test that forces the fix. @copilot please address this. |
||
| })); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/codebase-design]
AWF_INTERNAL_SIDECAR_HOSTSis duplicated acrossfirewall_blocked_domains.cjsandparse_firewall_logs.cjs— a future sidecar addition requires updating two files. Since both modules already require from the same directory, this list could live in a shared constant module (e.g.awf_internal_hosts.cjs) and be imported by both.💡 Example
This is a minor maintainability concern, not blocking, but worth addressing before the list grows.
@copilot please address this.