Skip to content

refactor: extract reportBlockedDomains helper to remove duplicated logic #2564

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
lpcox merged 5 commits into from
May 5, 2026
+163 −68
Merged

refactor: extract reportBlockedDomains helper to remove duplicated logic #2564

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b23c1b5
Initial plan
Copilot May 5, 2026
11babe4
refactor: extract reportBlockedDomains to remove duplicate logic
Copilot May 5, 2026
212d465
refactor: fix spelling categorised -> categorized in JSDoc
Copilot May 5, 2026
2889f4c
Merge remote-tracking branch 'origin/main' into copilot/fix-duplicate…
Copilot May 5, 2026
97239ab
fix: handle wildcards/protocol-prefixes, dedup suggestions, add tests
Copilot May 5, 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
120 changes: 52 additions & 68 deletions src/container-lifecycle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,56 @@ export async function startContainers(workDir: string, allowedDomains: string[],
}
}

/**
* Classifies and logs each blocked target, then emits actionable fix suggestions.
* Extracted to avoid duplicating this logic between the startup-error path
* (which uses `logger.error`) and the post-run warning path (which uses `logger.warn`).
*
* @param blockedTargets - Targets that were denied by the firewall
* @param allowedDomains - Domains currently in the allowlist
* @param log - Logging function to use (e.g. `logger.error` or `logger.warn`)
* @returns The categorized lists so callers can decide on further action
*/
function reportBlockedDomains(
blockedTargets: BlockedTarget[],
allowedDomains: string[],
log: (msg: string) => void,
): { missingDomains: string[]; portIssues: BlockedTarget[] } {
Comment on lines +522 to +526
const missingDomains: string[] = [];
const portIssues: BlockedTarget[] = [];

blockedTargets.forEach(blocked => {
const isAllowed = allowedDomains.some(allowed =>
blocked.domain === allowed || blocked.domain.endsWith('.' + allowed)
);

if (!isAllowed) {
// Domain not in allowlist
log(` - Blocked: ${blocked.target} (domain not in allowlist)`);
missingDomains.push(blocked.domain);
} else if (blocked.port && blocked.port !== '80' && blocked.port !== '443') {
// Domain is allowed but port is not
log(` - Blocked: ${blocked.target} (port ${blocked.port} not allowed, only 80 and 443 are permitted)`);
portIssues.push(blocked);
} else {
// Other reason (shouldn't happen often)
log(` - Blocked: ${blocked.target}`);
}
});

log('Allowed domains:');
allowedDomains.forEach(domain => { log(` - Allowed: ${domain}`); });

if (missingDomains.length > 0) {
log(`To fix domain issues: --allow-domains "${[...allowedDomains, ...missingDomains].join(',')}"`);
}
if (portIssues.length > 0) {
log('To fix port issues: Use standard ports 80 (HTTP) or 443 (HTTPS)');
}

return { missingDomains, portIssues };
}

/**
* Runs the Squid-log diagnostic check and re-throws with a user-friendly message
* when blocked domains are found, or rethrows the original error otherwise.
Expand All @@ -522,40 +572,7 @@ async function handleHealthcheckError(

if (hasDenials) {
logger.error('Firewall blocked domains during startup:');

const missingDomains: string[] = [];
const portIssues: BlockedTarget[] = [];

blockedTargets.forEach(blocked => {
const isAllowed = allowedDomains.some(allowed =>
blocked.domain === allowed || blocked.domain.endsWith('.' + allowed)
);

if (!isAllowed) {
// Domain not in allowlist
logger.error(` - Blocked: ${blocked.target} (domain not in allowlist)`);
missingDomains.push(blocked.domain);
} else if (blocked.port && blocked.port !== '80' && blocked.port !== '443') {
// Domain is allowed but port is not
logger.error(` - Blocked: ${blocked.target} (port ${blocked.port} not allowed, only 80 and 443 are permitted)`);
portIssues.push(blocked);
} else {
// Other reason (shouldn't happen often)
logger.error(` - Blocked: ${blocked.target}`);
}
});

logger.error('Allowed domains:');
allowedDomains.forEach(domain => {
logger.error(` - Allowed: ${domain}`);
});

if (missingDomains.length > 0) {
logger.error(`To fix domain issues: --allow-domains "${[...allowedDomains, ...missingDomains].join(',')}"`);
}
if (portIssues.length > 0) {
logger.error('To fix port issues: Use standard ports 80 (HTTP) or 443 (HTTPS)');
}
reportBlockedDomains(blockedTargets, allowedDomains, msg => logger.error(msg));

// Create a more user-friendly error
const blockedList = blockedTargets.map(b => `"${b.target}"`).join(', ');
Expand Down Expand Up @@ -642,40 +659,7 @@ export async function runAgentCommand(workDir: string, allowedDomains: string[],
// If command failed (non-zero exit) and domains were blocked, show a warning
if (exitCode !== 0 && hasDenials) {
logger.warn('Firewall blocked domains:');

const missingDomains: string[] = [];
const portIssues: BlockedTarget[] = [];

blockedTargets.forEach(blocked => {
const isAllowed = allowedDomains.some(allowed =>
blocked.domain === allowed || blocked.domain.endsWith('.' + allowed)
);

if (!isAllowed) {
// Domain not in allowlist
logger.warn(` - Blocked: ${blocked.target} (domain not in allowlist)`);
missingDomains.push(blocked.domain);
} else if (blocked.port && blocked.port !== '80' && blocked.port !== '443') {
// Domain is allowed but port is not
logger.warn(` - Blocked: ${blocked.target} (port ${blocked.port} not allowed, only 80 and 443 are permitted)`);
portIssues.push(blocked);
} else {
// Other reason (shouldn't happen often)
logger.warn(` - Blocked: ${blocked.target}`);
}
});

logger.warn('Allowed domains:');
allowedDomains.forEach(domain => {
logger.warn(` - Allowed: ${domain}`);
});

if (missingDomains.length > 0) {
logger.warn(`To fix domain issues: --allow-domains "${[...allowedDomains, ...missingDomains].join(',')}"`);
}
if (portIssues.length > 0) {
logger.warn('To fix port issues: Use standard ports 80 (HTTP) or 443 (HTTPS)');
}
reportBlockedDomains(blockedTargets, allowedDomains, msg => logger.warn(msg));
}

return { exitCode, blockedDomains: blockedTargets.map(b => b.domain) };
Expand Down
Loading