Fix firewall analysis inflating blocked count with internal Squid error entries#20137
Fix firewall analysis inflating blocked count with internal Squid error entries#20137
Conversation
…r entries (#issue)" Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes firewall log analysis incorrectly counting internal Squid error entries (localhost ::1 + placeholder domain/destination) as blocked external requests, which inflated blocked_requests and triggered false high-severity findings.
Changes:
- Filter out internal Squid error entries (
::1:*, domain-, dest-/-:-) in both Go and JS log parsers. - Tighten domain fallback logic to avoid treating
-:-as a real destination. - Add/extend tests to validate filtering behavior (Go + JS blocked-domain extractor).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/firewall_log.go | Skips internal Squid error entries and prevents -:- from being used as a domain fallback. |
| pkg/cli/firewall_log_test.go | Adds Go tests to ensure internal Squid entries are excluded from totals/domains. |
| actions/setup/js/parse_firewall_logs.cjs | Mirrors the Go filtering + fallback fix for the step-summary parser. |
| actions/setup/js/firewall_blocked_domains.cjs | Mirrors filtering + makes fallback explicitly exclude placeholder destinations. |
| actions/setup/js/firewall_blocked_domains.test.cjs | Adds a JS test to ensure internal Squid errors don’t affect blocked-domain extraction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
…ry filtering Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
💥 [THE END] — Illustrated by Smoke Claude
| } | ||
|
|
||
| // Skip internal Squid error entries (client IP ::1, no domain, no destination) | ||
| // These are internal Squid connection errors (e.g., error:transaction-end-before-headers) |
There was a problem hiding this comment.
Good defensive check for internal Squid error entries. The condition correctly identifies entries with ::1: client IP, no domain (-), and no destination (-:- or -). This prevents these internal errors from inflating blocked domain counts.
| const requestsByDomain = new Map(); | ||
|
|
||
| for (const line of lines) { | ||
| const entry = parseFirewallLogLine(line); |
There was a problem hiding this comment.
Nice refactoring - extracting analyzeFirewallLogLines as a separate function improves testability significantly. The function signature is clear and the return type is well-documented in the JSDoc comment.
pkg/cli/firewall_log.go: skip internal Squid error entries (::1:client, domain-, destIPPort-:-or-) before counting, and fix domain fallback to exclude-:-actions/setup/js/parse_firewall_logs.cjs: skip internal Squid error entries and fix domain fallback to exclude-:-actions/setup/js/firewall_blocked_domains.cjs: skip internal Squid error entries and fix domain fallback to explicitly exclude-:-pkg/cli/firewall_log_test.gofor internal Squid error entries being filteredactions/setup/js/firewall_blocked_domains.test.cjsfor internal Squid error entries being filteredanalyzeFirewallLogLinesfrommain()inparse_firewall_logs.cjsfor testabilityactions/setup/js/parse_firewall_logs.test.cjscovering internal Squid entry filtering in the main parsing loop (totals, per-domain stats,-:-domain fallback fix)💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
✨ PR Review Safe Output Test - Run 22833942676