diff --git a/.github/workflows/changeset-generator.firewall.lock.yml b/.github/workflows/changeset-generator.firewall.lock.yml index 4a7fe252df..dd10f31136 100644 --- a/.github/workflows/changeset-generator.firewall.lock.yml +++ b/.github/workflows/changeset-generator.firewall.lock.yml @@ -2992,23 +2992,71 @@ jobs: } + const timestamp = fields[0]; + + if (!/^\d+(\.\d+)?$/.test(timestamp)) { + + return null; + + } + + const clientIpPort = fields[1]; + + if (clientIpPort !== "-" && !/^[\d.]+:\d+$/.test(clientIpPort)) { + + return null; + + } + + const domain = fields[2]; + + if (domain !== "-" && !/^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)*:\d+$/.test(domain)) { + + return null; + + } + + const destIpPort = fields[3]; + + if (destIpPort !== "-" && !/^[\d.]+:\d+$/.test(destIpPort)) { + + return null; + + } + + const status = fields[6]; + + if (status !== "-" && !/^\d+$/.test(status)) { + + return null; + + } + + const decision = fields[7]; + + if (decision !== "-" && !decision.includes(":")) { + + return null; + + } + return { - timestamp: fields[0], + timestamp: timestamp, - clientIpPort: fields[1], + clientIpPort: clientIpPort, - domain: fields[2], + domain: domain, - destIpPort: fields[3], + destIpPort: destIpPort, proto: fields[4], method: fields[5], - status: fields[6], + status: status, - decision: fields[7], + decision: decision, url: fields[8], @@ -3050,19 +3098,25 @@ jobs: let summary = "# 🔥 Firewall Blocked Requests\n\n"; - if (deniedRequests > 0) { + const validDeniedDomains = deniedDomains.filter(domain => domain !== "-"); + + const validDeniedRequests = validDeniedDomains.reduce((sum, domain) => sum + (requestsByDomain.get(domain)?.denied || 0), 0); + + if (validDeniedRequests > 0) { + + summary += `**${validDeniedRequests}** request${validDeniedRequests !== 1 ? "s" : ""} blocked across **${validDeniedDomains.length}** unique domain${validDeniedDomains.length !== 1 ? "s" : ""}`; - summary += `**${deniedRequests}** request${deniedRequests !== 1 ? "s" : ""} blocked across **${deniedDomains.length}** unique domain${deniedDomains.length !== 1 ? "s" : ""}`; + summary += ` (${totalRequests > 0 ? Math.round((validDeniedRequests / totalRequests) * 100) : 0}% of total traffic)\n\n`; - summary += ` (${totalRequests > 0 ? Math.round((deniedRequests / totalRequests) * 100) : 0}% of total traffic)\n\n`; + summary += "
\n"; - summary += "## 🚫 Blocked Domains\n\n"; + summary += "🚫 Blocked Domains (click to expand)\n\n"; summary += "| Domain | Blocked Requests |\n"; summary += "|--------|------------------|\n"; - for (const domain of deniedDomains) { + for (const domain of validDeniedDomains) { const stats = requestsByDomain.get(domain); @@ -3070,7 +3124,7 @@ jobs: } - summary += "\n"; + summary += "\n
\n\n"; } else { diff --git a/.github/workflows/dev.firewall.lock.yml b/.github/workflows/dev.firewall.lock.yml index b2984bd400..61d7bacbcf 100644 --- a/.github/workflows/dev.firewall.lock.yml +++ b/.github/workflows/dev.firewall.lock.yml @@ -734,23 +734,71 @@ jobs: } + const timestamp = fields[0]; + + if (!/^\d+(\.\d+)?$/.test(timestamp)) { + + return null; + + } + + const clientIpPort = fields[1]; + + if (clientIpPort !== "-" && !/^[\d.]+:\d+$/.test(clientIpPort)) { + + return null; + + } + + const domain = fields[2]; + + if (domain !== "-" && !/^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)*:\d+$/.test(domain)) { + + return null; + + } + + const destIpPort = fields[3]; + + if (destIpPort !== "-" && !/^[\d.]+:\d+$/.test(destIpPort)) { + + return null; + + } + + const status = fields[6]; + + if (status !== "-" && !/^\d+$/.test(status)) { + + return null; + + } + + const decision = fields[7]; + + if (decision !== "-" && !decision.includes(":")) { + + return null; + + } + return { - timestamp: fields[0], + timestamp: timestamp, - clientIpPort: fields[1], + clientIpPort: clientIpPort, - domain: fields[2], + domain: domain, - destIpPort: fields[3], + destIpPort: destIpPort, proto: fields[4], method: fields[5], - status: fields[6], + status: status, - decision: fields[7], + decision: decision, url: fields[8], @@ -792,19 +840,25 @@ jobs: let summary = "# 🔥 Firewall Blocked Requests\n\n"; - if (deniedRequests > 0) { + const validDeniedDomains = deniedDomains.filter(domain => domain !== "-"); + + const validDeniedRequests = validDeniedDomains.reduce((sum, domain) => sum + (requestsByDomain.get(domain)?.denied || 0), 0); + + if (validDeniedRequests > 0) { + + summary += `**${validDeniedRequests}** request${validDeniedRequests !== 1 ? "s" : ""} blocked across **${validDeniedDomains.length}** unique domain${validDeniedDomains.length !== 1 ? "s" : ""}`; - summary += `**${deniedRequests}** request${deniedRequests !== 1 ? "s" : ""} blocked across **${deniedDomains.length}** unique domain${deniedDomains.length !== 1 ? "s" : ""}`; + summary += ` (${totalRequests > 0 ? Math.round((validDeniedRequests / totalRequests) * 100) : 0}% of total traffic)\n\n`; - summary += ` (${totalRequests > 0 ? Math.round((deniedRequests / totalRequests) * 100) : 0}% of total traffic)\n\n`; + summary += "
\n"; - summary += "## 🚫 Blocked Domains\n\n"; + summary += "🚫 Blocked Domains (click to expand)\n\n"; summary += "| Domain | Blocked Requests |\n"; summary += "|--------|------------------|\n"; - for (const domain of deniedDomains) { + for (const domain of validDeniedDomains) { const stats = requestsByDomain.get(domain); @@ -812,7 +866,7 @@ jobs: } - summary += "\n"; + summary += "\n
\n\n"; } else { diff --git a/.github/workflows/firewall.lock.yml b/.github/workflows/firewall.lock.yml index d4f005f557..c3c944938d 100644 --- a/.github/workflows/firewall.lock.yml +++ b/.github/workflows/firewall.lock.yml @@ -726,23 +726,71 @@ jobs: } + const timestamp = fields[0]; + + if (!/^\d+(\.\d+)?$/.test(timestamp)) { + + return null; + + } + + const clientIpPort = fields[1]; + + if (clientIpPort !== "-" && !/^[\d.]+:\d+$/.test(clientIpPort)) { + + return null; + + } + + const domain = fields[2]; + + if (domain !== "-" && !/^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)*:\d+$/.test(domain)) { + + return null; + + } + + const destIpPort = fields[3]; + + if (destIpPort !== "-" && !/^[\d.]+:\d+$/.test(destIpPort)) { + + return null; + + } + + const status = fields[6]; + + if (status !== "-" && !/^\d+$/.test(status)) { + + return null; + + } + + const decision = fields[7]; + + if (decision !== "-" && !decision.includes(":")) { + + return null; + + } + return { - timestamp: fields[0], + timestamp: timestamp, - clientIpPort: fields[1], + clientIpPort: clientIpPort, - domain: fields[2], + domain: domain, - destIpPort: fields[3], + destIpPort: destIpPort, proto: fields[4], method: fields[5], - status: fields[6], + status: status, - decision: fields[7], + decision: decision, url: fields[8], @@ -784,19 +832,25 @@ jobs: let summary = "# 🔥 Firewall Blocked Requests\n\n"; - if (deniedRequests > 0) { + const validDeniedDomains = deniedDomains.filter(domain => domain !== "-"); + + const validDeniedRequests = validDeniedDomains.reduce((sum, domain) => sum + (requestsByDomain.get(domain)?.denied || 0), 0); + + if (validDeniedRequests > 0) { + + summary += `**${validDeniedRequests}** request${validDeniedRequests !== 1 ? "s" : ""} blocked across **${validDeniedDomains.length}** unique domain${validDeniedDomains.length !== 1 ? "s" : ""}`; - summary += `**${deniedRequests}** request${deniedRequests !== 1 ? "s" : ""} blocked across **${deniedDomains.length}** unique domain${deniedDomains.length !== 1 ? "s" : ""}`; + summary += ` (${totalRequests > 0 ? Math.round((validDeniedRequests / totalRequests) * 100) : 0}% of total traffic)\n\n`; - summary += ` (${totalRequests > 0 ? Math.round((deniedRequests / totalRequests) * 100) : 0}% of total traffic)\n\n`; + summary += "
\n"; - summary += "## 🚫 Blocked Domains\n\n"; + summary += "🚫 Blocked Domains (click to expand)\n\n"; summary += "| Domain | Blocked Requests |\n"; summary += "|--------|------------------|\n"; - for (const domain of deniedDomains) { + for (const domain of validDeniedDomains) { const stats = requestsByDomain.get(domain); @@ -804,7 +858,7 @@ jobs: } - summary += "\n"; + summary += "\n
\n\n"; } else { diff --git a/.github/workflows/smoke-copilot.firewall.lock.yml b/.github/workflows/smoke-copilot.firewall.lock.yml index 567d7b0ae4..c1e4035405 100644 --- a/.github/workflows/smoke-copilot.firewall.lock.yml +++ b/.github/workflows/smoke-copilot.firewall.lock.yml @@ -2290,23 +2290,71 @@ jobs: } + const timestamp = fields[0]; + + if (!/^\d+(\.\d+)?$/.test(timestamp)) { + + return null; + + } + + const clientIpPort = fields[1]; + + if (clientIpPort !== "-" && !/^[\d.]+:\d+$/.test(clientIpPort)) { + + return null; + + } + + const domain = fields[2]; + + if (domain !== "-" && !/^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)*:\d+$/.test(domain)) { + + return null; + + } + + const destIpPort = fields[3]; + + if (destIpPort !== "-" && !/^[\d.]+:\d+$/.test(destIpPort)) { + + return null; + + } + + const status = fields[6]; + + if (status !== "-" && !/^\d+$/.test(status)) { + + return null; + + } + + const decision = fields[7]; + + if (decision !== "-" && !decision.includes(":")) { + + return null; + + } + return { - timestamp: fields[0], + timestamp: timestamp, - clientIpPort: fields[1], + clientIpPort: clientIpPort, - domain: fields[2], + domain: domain, - destIpPort: fields[3], + destIpPort: destIpPort, proto: fields[4], method: fields[5], - status: fields[6], + status: status, - decision: fields[7], + decision: decision, url: fields[8], @@ -2348,19 +2396,25 @@ jobs: let summary = "# 🔥 Firewall Blocked Requests\n\n"; - if (deniedRequests > 0) { + const validDeniedDomains = deniedDomains.filter(domain => domain !== "-"); + + const validDeniedRequests = validDeniedDomains.reduce((sum, domain) => sum + (requestsByDomain.get(domain)?.denied || 0), 0); + + if (validDeniedRequests > 0) { + + summary += `**${validDeniedRequests}** request${validDeniedRequests !== 1 ? "s" : ""} blocked across **${validDeniedDomains.length}** unique domain${validDeniedDomains.length !== 1 ? "s" : ""}`; - summary += `**${deniedRequests}** request${deniedRequests !== 1 ? "s" : ""} blocked across **${deniedDomains.length}** unique domain${deniedDomains.length !== 1 ? "s" : ""}`; + summary += ` (${totalRequests > 0 ? Math.round((validDeniedRequests / totalRequests) * 100) : 0}% of total traffic)\n\n`; - summary += ` (${totalRequests > 0 ? Math.round((deniedRequests / totalRequests) * 100) : 0}% of total traffic)\n\n`; + summary += "
\n"; - summary += "## 🚫 Blocked Domains\n\n"; + summary += "🚫 Blocked Domains (click to expand)\n\n"; summary += "| Domain | Blocked Requests |\n"; summary += "|--------|------------------|\n"; - for (const domain of deniedDomains) { + for (const domain of validDeniedDomains) { const stats = requestsByDomain.get(domain); @@ -2368,7 +2422,7 @@ jobs: } - summary += "\n"; + summary += "\n
\n\n"; } else { diff --git a/pkg/workflow/js/parse_firewall_logs.cjs b/pkg/workflow/js/parse_firewall_logs.cjs index 2487d45463..ccd80ce7e6 100644 --- a/pkg/workflow/js/parse_firewall_logs.cjs +++ b/pkg/workflow/js/parse_firewall_logs.cjs @@ -114,15 +114,51 @@ function parseFirewallLogLine(line) { return null; } + // Validate timestamp format (should be numeric with optional decimal point) + const timestamp = fields[0]; + if (!/^\d+(\.\d+)?$/.test(timestamp)) { + return null; + } + + // Validate client IP:port format (should be IP:port or "-") + const clientIpPort = fields[1]; + if (clientIpPort !== "-" && !/^[\d.]+:\d+$/.test(clientIpPort)) { + return null; + } + + // Validate domain format (should be domain:port or "-") + const domain = fields[2]; + if (domain !== "-" && !/^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)*:\d+$/.test(domain)) { + return null; + } + + // Validate dest IP:port format (should be IP:port or "-") + const destIpPort = fields[3]; + if (destIpPort !== "-" && !/^[\d.]+:\d+$/.test(destIpPort)) { + return null; + } + + // Validate status code (should be numeric or "-") + const status = fields[6]; + if (status !== "-" && !/^\d+$/.test(status)) { + return null; + } + + // Validate decision format (should contain ":" or be "-") + const decision = fields[7]; + if (decision !== "-" && !decision.includes(":")) { + return null; + } + return { - timestamp: fields[0], - clientIpPort: fields[1], - domain: fields[2], - destIpPort: fields[3], + timestamp: timestamp, + clientIpPort: clientIpPort, + domain: domain, + destIpPort: destIpPort, proto: fields[4], method: fields[5], - status: fields[6], - decision: fields[7], + status: status, + decision: decision, url: fields[8], userAgent: fields[9] ? fields[9].replace(/^"|"$/g, "") : "-", }; @@ -165,20 +201,27 @@ function generateFirewallSummary(analysis) { let summary = "# 🔥 Firewall Blocked Requests\n\n"; + // Filter out invalid domains (placeholder "-" values) + const validDeniedDomains = deniedDomains.filter(domain => domain !== "-"); + + // Calculate denied requests from valid domains only + const validDeniedRequests = validDeniedDomains.reduce((sum, domain) => sum + (requestsByDomain.get(domain)?.denied || 0), 0); + // Show blocked requests if any exist - if (deniedRequests > 0) { - summary += `**${deniedRequests}** request${deniedRequests !== 1 ? "s" : ""} blocked across **${deniedDomains.length}** unique domain${deniedDomains.length !== 1 ? "s" : ""}`; - summary += ` (${totalRequests > 0 ? Math.round((deniedRequests / totalRequests) * 100) : 0}% of total traffic)\n\n`; + if (validDeniedRequests > 0) { + summary += `**${validDeniedRequests}** request${validDeniedRequests !== 1 ? "s" : ""} blocked across **${validDeniedDomains.length}** unique domain${validDeniedDomains.length !== 1 ? "s" : ""}`; + summary += ` (${totalRequests > 0 ? Math.round((validDeniedRequests / totalRequests) * 100) : 0}% of total traffic)\n\n`; - summary += "## 🚫 Blocked Domains\n\n"; + summary += "
\n"; + summary += "🚫 Blocked Domains (click to expand)\n\n"; summary += "| Domain | Blocked Requests |\n"; summary += "|--------|------------------|\n"; - for (const domain of deniedDomains) { + for (const domain of validDeniedDomains) { const stats = requestsByDomain.get(domain); summary += `| ${domain} | ${stats.denied} |\n`; } - summary += "\n"; + summary += "\n
\n\n"; } else { summary += "✅ **No blocked requests detected**\n\n"; if (totalRequests > 0) { diff --git a/pkg/workflow/js/parse_firewall_logs.test.cjs b/pkg/workflow/js/parse_firewall_logs.test.cjs index 00a61da66e..a60dc554ff 100644 --- a/pkg/workflow/js/parse_firewall_logs.test.cjs +++ b/pkg/workflow/js/parse_firewall_logs.test.cjs @@ -55,9 +55,78 @@ describe("parse_firewall_logs.cjs", () => { expect(result.domain).toBe("api.enterprise.githubcopilot.com:443"); }); + test("should parse log line with placeholder values", () => { + const line = '1761332530.500 - - - - - 0 NONE_NONE:HIER_NONE - "-"'; + const result = parseFirewallLogLine(line); + expect(result).not.toBeNull(); + expect(result.domain).toBe("-"); + }); + test("should return null for empty line", () => { expect(parseFirewallLogLine("")).toBeNull(); }); + + test("should return null for invalid timestamp", () => { + expect( + parseFirewallLogLine( + 'WARNING: 172.30.0.20:35288 api.github.com:443 140.82.112.22:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.github.com:443 "-"' + ) + ).toBeNull(); + }); + + test("should return null for invalid client IP:port format", () => { + expect( + parseFirewallLogLine( + '1761332530.474 Accepting api.github.com:443 140.82.112.22:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.github.com:443 "-"' + ) + ).toBeNull(); + }); + + test("should return null for invalid domain format", () => { + expect( + parseFirewallLogLine( + '1761332530.474 172.30.0.20:35288 DNS 140.82.112.22:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.github.com:443 "-"' + ) + ).toBeNull(); + }); + + test("should return null for lines with fewer than 10 fields", () => { + expect(parseFirewallLogLine("WARNING: Something went wrong")).toBeNull(); + expect(parseFirewallLogLine("DNS lookup failed")).toBeNull(); + expect(parseFirewallLogLine("Accepting connection")).toBeNull(); + }); + + test("should return null for invalid dest IP:port format", () => { + expect( + parseFirewallLogLine( + '1761332530.474 172.30.0.20:35288 api.github.com:443 Local 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.github.com:443 "-"' + ) + ).toBeNull(); + }); + + test("should return null for invalid status code", () => { + expect( + parseFirewallLogLine( + '1761332530.474 172.30.0.20:35288 api.github.com:443 140.82.112.22:443 1.1 CONNECT Swap TCP_TUNNEL:HIER_DIRECT api.github.com:443 "-"' + ) + ).toBeNull(); + }); + + test("should return null for invalid decision format", () => { + expect( + parseFirewallLogLine( + '1761332530.474 172.30.0.20:35288 api.github.com:443 140.82.112.22:443 1.1 CONNECT 200 Waiting api.github.com:443 "-"' + ) + ).toBeNull(); + }); + + test("should return null for line with pipe character in domain position", () => { + expect( + parseFirewallLogLine( + '1761332530.474 172.30.0.20:35288 pinger|test 140.82.112.22:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.github.com:443 "-"' + ) + ).toBeNull(); + }); }); describe("isRequestAllowed", () => { @@ -98,8 +167,12 @@ describe("parse_firewall_logs.cjs", () => { expect(summary).toContain("**2** unique domains"); expect(summary).toContain("40% of total traffic"); + // Should wrap blocked domains in details tag + expect(summary).toContain("
"); + expect(summary).toContain("
"); + expect(summary).toContain("🚫 Blocked Domains (click to expand)"); + // Should show blocked domains table - expect(summary).toContain("🚫 Blocked Domains"); expect(summary).toContain("blocked.example.com:443"); expect(summary).toContain("denied.test.com:443"); @@ -108,6 +181,30 @@ describe("parse_firewall_logs.cjs", () => { expect(summary).not.toContain("api.github.com:443"); }); + test("should filter out placeholder domains", () => { + const analysis = { + totalRequests: 5, + allowedRequests: 2, + deniedRequests: 3, + allowedDomains: ["api.github.com:443"], + deniedDomains: ["-", "example.com:443"], + requestsByDomain: new Map([ + ["-", { allowed: 0, denied: 2 }], + ["example.com:443", { allowed: 0, denied: 1 }], + ]), + }; + + const summary = generateFirewallSummary(analysis); + + // Should only count valid domains + expect(summary).toContain("**1** request blocked"); + expect(summary).toContain("**1** unique domain"); + + // Should show example.com but not "-" + expect(summary).toContain("example.com:443"); + expect(summary).not.toContain("| - |"); + }); + test("should show success message when no blocked requests", () => { const analysis = { totalRequests: 3, @@ -123,5 +220,21 @@ describe("parse_firewall_logs.cjs", () => { expect(summary).toContain("✅ **No blocked requests detected**"); expect(summary).toContain("All 3 requests were allowed"); }); + + test("should show success message when only placeholder domains are blocked", () => { + const analysis = { + totalRequests: 3, + allowedRequests: 2, + deniedRequests: 1, + allowedDomains: ["api.github.com:443"], + deniedDomains: ["-"], + requestsByDomain: new Map([["-", { allowed: 0, denied: 1 }]]), + }; + + const summary = generateFirewallSummary(analysis); + + // Should show success when only invalid domains are blocked + expect(summary).toContain("✅ **No blocked requests detected**"); + }); }); });