Uh oh!
There was an error while loading. Please reload this page.
Fix firewall log parser rejecting invalid domains from Squid error messages - #2330
Conversation
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Agentic Changeset Generator triggered by this pull request. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the firewall log parser that was incorrectly accepting malformed Squid error messages as valid log entries, resulting in noise like "DNS", "WARNING:", and "Accepting" appearing as blocked domains. The fix adds strict field validation to reject invalid entries and filters placeholder domains from the summary output.
Key changes:
- Added regex validation for timestamp, IP:port, domain, status, and decision fields in
parseFirewallLogLine - Filtered out placeholder "-" domains from blocked domain statistics
- Wrapped blocked domains table in collapsible
<details>element for better UX
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/js/parse_firewall_logs.cjs | Added field format validation and placeholder domain filtering |
| pkg/workflow/js/parse_firewall_logs.test.cjs | Added test coverage for validation rules and placeholder filtering |
| .github/workflows/smoke-copilot.firewall.lock.yml | Applied same validation and filtering changes to workflow |
| .github/workflows/firewall.lock.yml | Applied same validation and filtering changes to workflow |
| .github/workflows/dev.firewall.lock.yml | Applied same validation and filtering changes to workflow |
| .github/workflows/changeset-generator.firewall.lock.yml | Applied same validation and filtering changes to workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let summary = "# 🔥 Firewall Blocked Requests\n\n"; | ||
| // Filter out invalid domains (placeholder "-" values) | ||
| const validDeniedDomains = deniedDomains.filter(domain => domain !== "-"); |
There was a problem hiding this comment.
Use arrow function syntax consistently with parentheses around the parameter for single-parameter arrow functions to match project style.
There was a problem hiding this comment.
Fixed in e34d69b - added parentheses around the parameter for consistency.
| let validDeniedRequests = 0; | ||
| for (const domain of validDeniedDomains) { | ||
| const stats = requestsByDomain.get(domain); | ||
| if (stats) { | ||
| validDeniedRequests += stats.denied; | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] This loop can be simplified using reduce for a more functional approach: const validDeniedRequests = validDeniedDomains.reduce((sum, domain) => sum + (requestsByDomain.get(domain)?.denied || 0), 0);
| letvalidDeniedRequests=0; | |
| for(constdomainofvalidDeniedDomains){ | |
| conststats=requestsByDomain.get(domain); | |
| if(stats){ | |
| validDeniedRequests+=stats.denied; | |
| } | |
| } | |
| constvalidDeniedRequests=validDeniedDomains.reduce( | |
| (sum,domain)=>sum+(requestsByDomain.get(domain)?.denied||0), | |
| 0 | |
| ); |
There was a problem hiding this comment.
Implemented in e34d69b - simplified to use reduce with optional chaining for a more functional approach.
| // Validate domain format (should be domain:port or "-") | ||
| const domain = fields[2]; | ||
| if (domain !== "-" && !/^[a-zA-Z0-9.-]+:\d+$/.test(domain)) { |
There was a problem hiding this comment.
The domain regex allows consecutive dots (e.g., "example..com:443") and leading/trailing dots (e.g., ".example.com:443"), which are invalid domain formats. Consider using a more strict pattern like /^[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+$/ to enforce valid domain structure.
| if(domain!=="-"&&!/^[a-zA-Z0-9.-]+:\d+$/.test(domain)){ | |
| 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) | |
| ){ |
There was a problem hiding this comment.
Fixed in e34d69b - implemented the stricter domain validation pattern that prevents consecutive dots, leading/trailing dots, and invalid label structures. Validated with comprehensive edge case testing.
Mossaka
commented
Oct 24, 2025
@copilot rebase and regenerate all the yamls using |
Mossaka
commented
Oct 24, 2025
@copilot apply changes based on the comments in this thread |
…al approach Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
All changes from that review thread have already been applied in commits 58b2d9b and e34d69b:
The PR is ready for review. |
Uh oh!
There was an error while loading. Please reload this page.
Fix Blocked Domains Parsing and Display
This PR addresses the issue where the firewall log parser was showing invalid "domains" like "DNS", "Local", "Swap", "WARNING:", etc. in the blocked domains table.
Latest Changes
Recompiled workflows with updated firewall log parsing logic.
Code Review Fixes
reducefor a more functional approachOriginal Implementation
Strict field validation in
parseFirewallLogLine:/^\d+(\.\d+)?$//^[\d.]+:\d+$/or---:or-Filter placeholder domains in summary:
UI enhancement:
<details>tag for collapsibility<summary>with "click to expand" promptDomain Validation Examples
✅ Valid domains:
api.github.com:443sub.example.com:443my-domain.com:443❌ Rejected invalid domains:
.example.com:443(leading dot)example.com.:443(trailing dot)example..com:443(consecutive dots)-example.com:443(leading hyphen)example.com-:443(trailing hyphen in label)Testing
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.