-
Notifications
You must be signed in to change notification settings - Fork 46
Fix firewall log parser rejecting invalid domains from Squid error messages #2330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 !== "-"); |
Copilot
AI
Oct 24, 2025
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; | ||
| } | ||
| } |
Copilot
AI
Oct 24, 2025
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.
[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);
| let validDeniedRequests = 0; | |
| for (const domain of validDeniedDomains) { | |
| const stats = requestsByDomain.get(domain); | |
| if (stats) { | |
| validDeniedRequests += stats.denied; | |
| } | |
| } | |
| const validDeniedRequests = validDeniedDomains.reduce( | |
| (sum, domain) => sum + (requestsByDomain.get(domain)?.denied || 0), | |
| 0 | |
| ); |
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.
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)) { |
Copilot
AI
Oct 24, 2025
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
@copilot rebase and regenerate all the yamls using |
|
@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. |
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.