Skip to content

Enhance firewall blocked domains footer with fix snippet and docs link #18676

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.

Sign up for GitHub

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

Merged
pelikhan merged 2 commits into from
Feb 27, 2026
Merged

Enhance firewall blocked domains footer with fix snippet and docs link #18676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef3ab48
Initial plan
Copilot Feb 27, 2026
ab4b40a
Add frontmatter snippet and docs link to firewall blocked domains foo…
Copilot Feb 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions actions/setup/js/firewall_blocked_domains.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,19 @@ function generateBlockedDomainsSection(blockedDomains) {
section += `> - \`${domain}\`\n`;
}

section += `>\n`;
section += `> To allow these domains, add them to the \`network.allowed\` list in your workflow frontmatter:\n`;
section += `>\n`;
Comment on lines +196 to +197
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text says "To allow these domains" even when only a single blocked domain is listed. This will produce grammatically incorrect guidance in the 1-domain case; please make the wording conditional (e.g., "this domain" vs "these domains").

Copilot uses AI. Check for mistakes.
section += `> \`\`\`yaml\n`;
section += `> network:\n`;
section += `> allowed:\n`;
section += `> - defaults\n`;
for (const domain of blockedDomains) {
section += `> - "${domain}"\n`;
}
Comment on lines +201 to +204
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The YAML snippet uses the same blockedDomains values that come from extractAndSanitizeDomain(), which calls sanitizeDomainName() (actions/setup/js/sanitize_content_core.cjs:146+). That sanitizer removes valid hostname characters like - (e.g., "my-domain.co.uk" → "mydomain.co.uk") and can truncate long domains, so the suggested network.allowed entries may not match the actual blocked host and may not resolve the firewall block when copy/pasted. Consider keeping the raw hostname (minus port) for the config snippet (with proper escaping), while separately sanitizing only for display if needed.

Copilot uses AI. Check for mistakes.
section += `> \`\`\`\n`;
section += `>\n`;
section += `> See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information.\n`;
section += `>\n`;
section += `> </details>\n`;

Expand Down
4 changes: 4 additions & 0 deletions actions/setup/js/firewall_blocked_domains.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,8 @@ describe("firewall_blocked_domains.cjs", () => {
expect(result).toContain("> <summary>⚠️ Firewall blocked 1 domain</summary>");
expect(result).toContain("> - `blocked.example.com`");
expect(result).toContain("> The following domain was blocked by the firewall during workflow execution:");
expect(result).toContain('> ```yaml\n> network:\n> allowed:\n> - defaults\n> - "blocked.example.com"\n> ```');
expect(result).toContain("> See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information.");
Comment on lines +266 to +267
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These assertions match the entire multi-line YAML block as an exact substring, which makes the test brittle to harmless formatting changes (indentation, trailing newline, etc.). Consider asserting key lines separately or using a regex/snapshot so the test focuses on structure/content rather than exact whitespace.

This issue also appears on line 281 of the same file.

Copilot uses AI. Check for mistakes.
});

it("should generate details section for multiple blocked domains", () => {
Expand All @@ -276,6 +278,8 @@ describe("firewall_blocked_domains.cjs", () => {
expect(result).toContain("> - `alpha.example.com`");
expect(result).toContain("> - `beta.example.com`");
expect(result).toContain("> - `gamma.example.com`");
expect(result).toContain('> ```yaml\n> network:\n> allowed:\n> - defaults\n> - "alpha.example.com"\n> - "beta.example.com"\n> - "gamma.example.com"\n> ```');
expect(result).toContain("> See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information.");
});

it("should use correct singular/plural form", () => {
Expand Down