-
Notifications
You must be signed in to change notification settings - Fork 136
Add firewall blocked domains to AI-generated footers #14517
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: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit 43a6f48:
The firewall logs are uploaded as part of Visual change: Example: <details>
<summary>⚠️ Firewall blocked 2 domains</summary>
> The following domains were blocked by the firewall during workflow execution:
- `blocked.example.com`
- `malicious.site`
</details> |
|
@copilot the details element should be in a warning alert (GitHub markdown syntax) |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit 006ef9b. The details element is now wrapped in a GitHub warning alert using the Example output: > [!WARNING]
> <details>
> <summary>⚠️ Firewall blocked 2 domains</summary>
>
> The following domains were blocked by the firewall during workflow execution:
>
> - `blocked.example.com`
> - `malicious.site`
>
> </details>The warning alert provides:
|
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
Adds firewall-log awareness to AI-generated footers so users can see (in a collapsed warning block) which domains were blocked by the network firewall during workflow execution.
Changes:
- Add
firewall_blocked_domains.cjshelper to parse firewall logs, sanitize domains, and render a GitHub warning-alert<details>section. - Integrate the blocked-domains footer section into both
generate_footer.cjsandmessages_footer.cjs. - Add tests for the new module and a regression test ensuring no section is emitted when no logs are present; add a workflow markdown file for manual validation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/workflows/test-firewall-blocked-domains-footer.md | Adds a manual test workflow for validating the blocked-domains footer output. |
| actions/setup/js/messages_footer.cjs | Appends blocked-domains warning section to message-based footer generation. |
| actions/setup/js/generate_footer.test.cjs | Adds a test asserting the blocked-domains section is omitted when no logs exist. |
| actions/setup/js/generate_footer.cjs | Appends blocked-domains warning section to the standard footer generation path. |
| actions/setup/js/firewall_blocked_domains.test.cjs | Adds unit tests covering parsing, blocked/allowed detection, sanitization, aggregation, and rendering. |
| actions/setup/js/firewall_blocked_domains.cjs | New module implementing log discovery/parsing + rendering of the warning <details> section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if request was blocked | ||
| const isBlocked = isRequestBlocked(entry.decision, entry.status); | ||
| if (isBlocked) { | ||
| const sanitizedDomain = extractAndSanitizeDomain(entry.domain); | ||
| if (sanitizedDomain && sanitizedDomain !== "-") { | ||
| blockedDomainsSet.add(sanitizedDomain); | ||
| } | ||
| } |
Copilot
AI
Feb 8, 2026
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.
In getBlockedDomains(), Envoy-style pseudo-domains like error:transaction-end-before-headers will currently be treated as real domains (e.g., truncated to error) and could show up in the footer. The existing firewall log summary code filters out placeholder - and any domain starting with error:; this footer extraction should apply the same filtering before sanitizing/adding to the set so the footer only lists actual blocked domains.
| function parseFirewallLogLine(line) { | ||
| const trimmed = line.trim(); | ||
| if (!trimmed || trimmed.startsWith("#")) { | ||
| return null; | ||
| } | ||
|
|
||
| // Split by whitespace but preserve quoted strings | ||
| const fields = trimmed.match(/(?:[^\s"]+|"[^"]*")+/g); | ||
| if (!fields || fields.length < 10) { | ||
| return null; | ||
| } | ||
|
|
||
| // Only validate timestamp (essential for log format detection) | ||
| const timestamp = fields[0]; | ||
| if (!/^\d+(\.\d+)?$/.test(timestamp)) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| timestamp, | ||
| clientIpPort: fields[1], | ||
| domain: fields[2], | ||
| destIpPort: fields[3], | ||
| proto: fields[4], | ||
| method: fields[5], | ||
| status: fields[6], | ||
| decision: fields[7], | ||
| url: fields[8], | ||
| userAgent: fields[9]?.replace(/^"|"$/g, "") || "-", | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Determines if a request was blocked based on decision and status | ||
| * @param {string} decision - Decision field (e.g., TCP_TUNNEL:HIER_DIRECT, NONE_NONE:HIER_NONE) | ||
| * @param {string} status - Status code (e.g., 200, 403, 0) | ||
| * @returns {boolean} True if request was blocked | ||
| */ | ||
| function isRequestBlocked(decision, status) { | ||
| // Check status code first | ||
| const statusCode = parseInt(status, 10); | ||
| if (statusCode === 403 || statusCode === 407) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check decision field | ||
| if (decision.includes("NONE_NONE") || decision.includes("TCP_DENIED")) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check for allowed indicators | ||
| if (statusCode === 200 || statusCode === 206 || statusCode === 304) { | ||
| return false; | ||
| } | ||
|
|
||
| if (decision.includes("TCP_TUNNEL") || decision.includes("TCP_HIT") || decision.includes("TCP_MISS")) { | ||
| return false; | ||
| } | ||
|
|
||
| // Default to blocked for safety | ||
| return true; |
Copilot
AI
Feb 8, 2026
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.
This module duplicates the firewall log parsing and allow/deny classification logic that already exists in actions/setup/js/parse_firewall_logs.cjs (parseFirewallLogLine + decision/status checks). To avoid the two implementations diverging over time, consider extracting shared helpers (e.g., a small firewall_log_core.cjs) or importing/reusing the existing functions where possible.
| it("should not include blocked domains section when no firewall logs exist", () => { | ||
| const result = generateFooter("Test Workflow", "https://github.com/test/repo/actions/runs/123", "", "", undefined, undefined, undefined); | ||
|
|
||
| expect(result).not.toContain("⚠️ Firewall blocked"); | ||
| expect(result).not.toContain("<details>"); | ||
| }); |
Copilot
AI
Feb 8, 2026
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.
This test assumes /tmp/gh-aw/sandbox/firewall/logs/ does not exist and contains no .log files. Since other tests in this repo create /tmp/gh-aw, this assertion can become environment-dependent/flaky. Make the test deterministic by explicitly removing/isolating the firewall logs path in setup/teardown or by mocking getBlockedDomains() to return an empty list for this case.
| testDir = path.join(os.tmpdir(), `gh-aw-test-firewall-${Date.now()}`); | ||
| fs.mkdirSync(testDir, { recursive: true }); |
Copilot
AI
Feb 8, 2026
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 temporary directory name uses only Date.now(), which is not guaranteed to be unique when Vitest runs test files in parallel. Use a truly unique temp dir (e.g., fs.mkdtempSync() or include pid/random) to avoid rare collisions between concurrent test workers.
| testDir = path.join(os.tmpdir(), `gh-aw-test-firewall-${Date.now()}`); | |
| fs.mkdirSync(testDir, { recursive: true }); | |
| testDir = fs.mkdtempSync(path.join(os.tmpdir(), "gh-aw-test-firewall-")); |
Add Firewall Blocked Domains to AI-Generated Footers ✅
Overview
When creating AI-generated footers on items (issues, PRs, comments), the footer now reviews firewall logs and adds a collapsed details message if any domains were blocked. Inside the details section, it shows a list of sanitized domains to inform users about blocked domain access attempts.
The blocked domains section is wrapped in a GitHub warning alert for enhanced visibility and prominence.
Recent Updates (addressing feedback)
Changes made:
> [!WARNING])The firewall logs are uploaded as part of
agent-artifactsand downloaded to/tmp/gh-aw/sandbox/firewall/logs/in the safe-outputs job, which is the same default path used by the module. The implementation works correctly in both contexts:Example Output
When rendered on GitHub, this appears as a yellow/orange warning box with:
Implementation Status: COMPLETE ✅
All checklist items completed and feedback addressed:
generate_footer.cjsandmessages_footer.cjs)Files Changed
New Files:
actions/setup/js/firewall_blocked_domains.cjs(201 lines)actions/setup/js/firewall_blocked_domains.test.cjs(367 lines)pkg/cli/workflows/test-firewall-blocked-domains-footer.md(61 lines)Modified Files:
actions/setup/js/generate_footer.cjsactions/setup/js/generate_footer.test.cjsactions/setup/js/messages_footer.cjsTest Results
JavaScript Tests: ✅
Code Quality: ✅
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.