Refactor squid config generation into focused modules#3028
Conversation
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 87.63% | 89.86% | 📈 +2.23% |
| Statements | 87.60% | 89.77% | 📈 +2.17% |
| Functions | 92.16% | 92.02% | 📉 -0.14% |
| Branches | 79.65% | 81.76% | 📈 +2.11% |
📁 Per-file Coverage Changes (4 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/commands/main-action.ts |
5.7% → 7.2% (+1.49%) | 5.7% → 7.1% (+1.45%) |
src/host-iptables.ts |
97.7% → 100.0% (+2.34%) | 97.3% → 100.0% (+2.70%) |
src/squid-config.ts |
97.1% → 100.0% (+2.90%) | 97.2% → 100.0% (+2.85%) |
src/container-lifecycle.ts |
87.4% → 93.9% (+6.57%) | 87.7% → 94.2% (+6.50%) |
✨ New Files (12 files)
src/config-writer.ts: 81.0% linessrc/host-iptables-cleanup.ts: 100.0% linessrc/host-iptables-network.ts: 100.0% linessrc/host-iptables-rules.ts: 97.3% linessrc/host-iptables-shared.ts: 96.5% linessrc/commands/network-setup.ts: 100.0% linessrc/commands/preflight.ts: 100.0% linessrc/commands/signal-handler.ts: 89.5% linessrc/squid/domain-acl.ts: 100.0% linessrc/squid/policy-manifest.ts: 87.0% linessrc/squid/ssl-bump.ts: 93.8% linessrc/squid/upstream-proxy.ts: 100.0% lines
Coverage comparison generated by scripts/ci/compare-coverage.ts
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Refactors Squid config generation by extracting domain ACL parsing, SSL bump section generation, upstream-proxy chaining, and policy-manifest generation into focused modules, while keeping generateSquidConfig/generatePolicyManifest available via the src/squid-config.ts facade.
Changes:
- Extracted domain parsing/formatting + Squid interpolation safety helpers into
src/squid/domain-acl.ts. - Extracted SSL bump config assembly into
src/squid/ssl-bump.tsand upstream proxy chaining intosrc/squid/upstream-proxy.ts. - Moved policy manifest generation (and
DANGEROUS_PORTS) intosrc/squid/policy-manifest.ts, re-exported fromsrc/squid-config.ts.
Show a summary per file
| File | Description |
|---|---|
| src/squid-config.ts | Converts to a facade/orchestrator importing extracted helpers and re-exporting generatePolicyManifest. |
| src/squid/domain-acl.ts | New shared domain parsing/grouping + Squid-safe interpolation utilities. |
| src/squid/ssl-bump.ts | New module generating the SSL bump section (including optional URL-pattern ACL/rules). |
| src/squid/upstream-proxy.ts | New module generating cache_peer + bypass rules for upstream proxy chaining. |
| src/squid/policy-manifest.ts | New module generating the structured policy manifest and exporting DANGEROUS_PORTS. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 3
| // Build access rules for URL patterns | ||
| // When URL patterns are specified, we: | ||
| // 1. Allow requests matching the URL patterns | ||
| // 2. Deny all other requests to allowed_domains (they didn't match URL patterns) | ||
| const urlAccessLines = urlPatterns | ||
| .map((_, i) => `http_access allow allowed_url_${i}`) | ||
| .join('\n'); | ||
|
|
||
| // Deny requests to allowed domains that don't match URL patterns | ||
| // This ensures URL-level filtering is enforced | ||
| // IMPORTANT: Use !CONNECT to only deny actual HTTP requests after bump, | ||
| // not the CONNECT request itself (which must be allowed for SSL bump to work) | ||
| const denyNonMatching = hasPlainDomains | ||
| ? 'http_access deny !CONNECT allowed_domains' | ||
| : hasPatterns | ||
| ? 'http_access deny !CONNECT allowed_domains_regex' | ||
| : ''; | ||
|
|
||
| urlAccessRules = `\n# Allow HTTPS requests matching URL patterns\n${urlAccessLines}\n\n# Deny requests that don't match URL patterns\n${denyNonMatching}\n`; |
| /** | ||
| * Defense-in-depth: assert a domain/regex/URL-pattern string is safe for Squid config interpolation. | ||
| * Rejects whitespace, null bytes, quotes, semicolons, backticks, hash characters, and backslashes — | ||
| * all of which can inject directives, tokens, or comments into Squid config. | ||
| */ | ||
| export function assertSafeForSquidConfig(value: string): string { | ||
| if (SQUID_DANGEROUS_CHARS.test(value)) { | ||
| throw new Error( | ||
| `SECURITY: Domain or pattern contains characters unsafe for Squid config and cannot be ` + | ||
| `interpolated into squid.conf: ${JSON.stringify(value)}` | ||
| ); |
| export function generateUpstreamProxySection(upstream: UpstreamProxyConfig): string { | ||
| const lines: string[] = [ | ||
| '# Upstream corporate proxy — route outbound traffic through parent proxy', | ||
| '# Required for self-hosted runners where direct egress is blocked', | ||
| `cache_peer ${upstream.host} parent ${upstream.port} 0 no-query default`, | ||
| ]; | ||
|
|
||
| // Generate always_direct ACL for no_proxy bypass domains | ||
| if (upstream.noProxy && upstream.noProxy.length > 0) { | ||
| lines.push(''); | ||
| lines.push('# Bypass upstream proxy for these domains (from host no_proxy)'); | ||
| for (const domain of upstream.noProxy) { | ||
| // All entries are treated as suffix matches (domain + subdomains), | ||
| // matching standard no_proxy semantics: | ||
| // .corp.com → *.corp.com | ||
| // internal.corp.com → internal.corp.com AND *.internal.corp.com | ||
| const squidDomain = domain.startsWith('.') ? domain : `.${domain}`; | ||
| lines.push(`acl upstream_bypass dstdomain ${squidDomain}`); | ||
| // For non-dot entries, also add the exact domain for Squid dstdomain matching | ||
| if (!domain.startsWith('.')) { | ||
| lines.push(`acl upstream_bypass dstdomain ${domain}`); | ||
| } |
|
@copilot address the review feedback |
Addressed in commit
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@copilot fix this failing ci check https://github.com/github/gh-aw-firewall/actions/runs/25750190383/job/75625642186?pr=3028 |
Fixed in |
🔬 Smoke Test Results
Overall: FAIL — Workflow template variables (
|
Smoke Test: Copilot BYOK (Offline) Mode
Running in BYOK offline mode ( Overall: FAIL — pre-step smoke data was not injected into the prompt (template variables unresolved) and GitHub MCP returned 401.
|
|
Smoke Codex: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Chroot Smoke Test Results
Overall: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot.
|
Smoke Test Results
Overall: FAIL — service containers are not reachable from this environment.
|
Smoke Test Results✅ GitHub commits verification Overall: PASS
|
✨ Enhancement
src/squid-config.tshad grown into a single 945-line module mixing domain ACL parsing, SSL bump generation, upstream proxy chaining, and policy-manifest construction. This refactor decomposes those concerns into focused modules while preserving the existinggenerateSquidConfig/generatePolicyManifestpublic surface.What does this improve?
Why is this valuable?
src/container-lifecycle.tscontinues importing fromsrc/squid-config.ts).Implementation approach:
src/squid/domain-acl.ts: extracted domain parsing/grouping and Squid interpolation safety helpers.src/squid/ssl-bump.ts: extracted SSL bump section assembly.src/squid/upstream-proxy.ts: extracted upstream proxycache_peer/ bypass section generation.src/squid/policy-manifest.ts: extracted policy-manifest generation andDANGEROUS_PORTS.src/squid-config.tsnow focuses ongenerateSquidConfigorchestration and imports extracted helpers.generatePolicyManifestremains available viasrc/squid-config.tsthrough re-export for compatibility.