Skip to content

Commit b81351c

Browse files
CopilotCopilot
authored
refactor: extract reportBlockedDomains helper to remove duplicated logic (#2564)
* Initial plan * refactor: extract reportBlockedDomains to remove duplicate logic * refactor: fix spelling categorised -> categorized in JSDoc Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/a4ea6ef3-71ec-4c40-821b-80aaf7378c48 * fix: handle wildcards/protocol-prefixes, dedup suggestions, add tests --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
1 parent b2d2923 commit b81351c

2 files changed

Lines changed: 163 additions & 68 deletions

File tree

‎src/container-lifecycle.ts‎

Lines changed: 65 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import execa from 'execa';
55
import { WrapperConfig, BlockedTarget } from './types';
66
import { logger } from './logger';
77
import { generateSquidConfig, generatePolicyManifest } from './squid-config';
8+
import { parseDomainWithProtocol, isWildcardPattern, wildcardToRegex } from './domain-patterns';
89
import { generateSessionCa, initSslDb, parseUrlPatterns } from './ssl-bump';
910
import {
1011
SQUID_PORT,
@@ -508,6 +509,68 @@ export async function startContainers(workDir: string, allowedDomains: string[],
508509
}
509510
}
510511

512+
/**
513+
* Classifies and logs each blocked target, then emits actionable fix suggestions.
514+
* Extracted to avoid duplicating this logic between the startup-error path
515+
* (which uses `logger.error`) and the post-run warning path (which uses `logger.warn`).
516+
*
517+
* @param blockedTargets - Targets that were denied by the firewall
518+
* @param allowedDomains - Domains currently in the allowlist
519+
* @param log - Logging function to use (e.g. `logger.error` or `logger.warn`)
520+
* @returns The categorized lists so callers can decide on further action
521+
*/
522+
function reportBlockedDomains(
523+
blockedTargets: BlockedTarget[],
524+
allowedDomains: string[],
525+
log: (msg: string) => void,
526+
): { missingDomains: string[]; portIssues: BlockedTarget[] } {
527+
const uniqueMissingDomains = new Set<string>();
528+
const portIssues: BlockedTarget[] = [];
529+
530+
blockedTargets.forEach(blocked => {
531+
const isAllowed = allowedDomains.some(allowed => {
532+
// Strip any protocol prefix (e.g. "https://github.com" -> "github.com")
533+
const normalizedAllowed = parseDomainWithProtocol(allowed).domain;
534+
if (isWildcardPattern(normalizedAllowed)) {
535+
// Wildcard pattern match (e.g. "*.github.com")
536+
try {
537+
return new RegExp(wildcardToRegex(normalizedAllowed), 'i').test(blocked.domain);
538+
} catch {
539+
return false;
540+
}
541+
}
542+
// Exact match or subdomain match
543+
return blocked.domain === normalizedAllowed || blocked.domain.endsWith('.' + normalizedAllowed);
544+
});
545+
546+
if (!isAllowed) {
547+
// Domain not in allowlist
548+
log(` - Blocked: ${blocked.target} (domain not in allowlist)`);
549+
uniqueMissingDomains.add(blocked.domain);
550+
} else if (blocked.port && blocked.port !== '80' && blocked.port !== '443') {
551+
// Domain is allowed but port is not
552+
log(` - Blocked: ${blocked.target} (port ${blocked.port} not allowed, only 80 and 443 are permitted)`);
553+
portIssues.push(blocked);
554+
} else {
555+
// Other reason (shouldn't happen often)
556+
log(` - Blocked: ${blocked.target}`);
557+
}
558+
});
559+
560+
log('Allowed domains:');
561+
allowedDomains.forEach(domain => { log(` - Allowed: ${domain}`); });
562+
563+
const missingDomains = [...uniqueMissingDomains];
564+
if (missingDomains.length > 0) {
565+
log(`To fix domain issues: --allow-domains "${[...allowedDomains, ...missingDomains].join(',')}"`);
566+
}
567+
if (portIssues.length > 0) {
568+
log('To fix port issues: Use standard ports 80 (HTTP) or 443 (HTTPS)');
569+
}
570+
571+
return { missingDomains, portIssues };
572+
}
573+
511574
/**
512575
* Runs the Squid-log diagnostic check and re-throws with a user-friendly message
513576
* when blocked domains are found, or rethrows the original error otherwise.
@@ -524,40 +587,7 @@ async function handleHealthcheckError(
524587

525588
if (hasDenials) {
526589
logger.error('Firewall blocked domains during startup:');
527-
528-
const missingDomains: string[] = [];
529-
const portIssues: BlockedTarget[] = [];
530-
531-
blockedTargets.forEach(blocked => {
532-
const isAllowed = allowedDomains.some(allowed =>
533-
blocked.domain === allowed || blocked.domain.endsWith('.' + allowed)
534-
);
535-
536-
if (!isAllowed) {
537-
// Domain not in allowlist
538-
logger.error(` - Blocked: ${blocked.target} (domain not in allowlist)`);
539-
missingDomains.push(blocked.domain);
540-
} else if (blocked.port && blocked.port !== '80' && blocked.port !== '443') {
541-
// Domain is allowed but port is not
542-
logger.error(` - Blocked: ${blocked.target} (port ${blocked.port} not allowed, only 80 and 443 are permitted)`);
543-
portIssues.push(blocked);
544-
} else {
545-
// Other reason (shouldn't happen often)
546-
logger.error(` - Blocked: ${blocked.target}`);
547-
}
548-
});
549-
550-
logger.error('Allowed domains:');
551-
allowedDomains.forEach(domain => {
552-
logger.error(` - Allowed: ${domain}`);
553-
});
554-
555-
if (missingDomains.length > 0) {
556-
logger.error(`To fix domain issues: --allow-domains "${[...allowedDomains, ...missingDomains].join(',')}"`);
557-
}
558-
if (portIssues.length > 0) {
559-
logger.error('To fix port issues: Use standard ports 80 (HTTP) or 443 (HTTPS)');
560-
}
590+
reportBlockedDomains(blockedTargets, allowedDomains, msg => logger.error(msg));
561591

562592
// Create a more user-friendly error
563593
const blockedList = blockedTargets.map(b => `"${b.target}"`).join(', ');
@@ -644,40 +674,7 @@ export async function runAgentCommand(workDir: string, allowedDomains: string[],
644674
// If command failed (non-zero exit) and domains were blocked, show a warning
645675
if (exitCode !== 0 && hasDenials) {
646676
logger.warn('Firewall blocked domains:');
647-
648-
const missingDomains: string[] = [];
649-
const portIssues: BlockedTarget[] = [];
650-
651-
blockedTargets.forEach(blocked => {
652-
const isAllowed = allowedDomains.some(allowed =>
653-
blocked.domain === allowed || blocked.domain.endsWith('.' + allowed)
654-
);
655-
656-
if (!isAllowed) {
657-
// Domain not in allowlist
658-
logger.warn(` - Blocked: ${blocked.target} (domain not in allowlist)`);
659-
missingDomains.push(blocked.domain);
660-
} else if (blocked.port && blocked.port !== '80' && blocked.port !== '443') {
661-
// Domain is allowed but port is not
662-
logger.warn(` - Blocked: ${blocked.target} (port ${blocked.port} not allowed, only 80 and 443 are permitted)`);
663-
portIssues.push(blocked);
664-
} else {
665-
// Other reason (shouldn't happen often)
666-
logger.warn(` - Blocked: ${blocked.target}`);
667-
}
668-
});
669-
670-
logger.warn('Allowed domains:');
671-
allowedDomains.forEach(domain => {
672-
logger.warn(` - Allowed: ${domain}`);
673-
});
674-
675-
if (missingDomains.length > 0) {
676-
logger.warn(`To fix domain issues: --allow-domains "${[...allowedDomains, ...missingDomains].join(',')}"`);
677-
}
678-
if (portIssues.length > 0) {
679-
logger.warn('To fix port issues: Use standard ports 80 (HTTP) or 443 (HTTPS)');
680-
}
677+
reportBlockedDomains(blockedTargets, allowedDomains, msg => logger.warn(msg));
681678
}
682679

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

‎src/docker-manager-lifecycle.test.ts‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { startContainers, stopContainers, fastKillAgentContainer, isAgentExternallyKilled, resetAgentExternallyKilled, AGENT_CONTAINER_NAME, runAgentCommand, setAwfDockerHost } from './docker-manager';
2+
import { logger } from './logger';
23
import * as fs from 'fs';
34
import * as path from 'path';
45
import * as os from 'os';
@@ -594,5 +595,102 @@ describe('docker-manager lifecycle', () => {
594595
expect(result.exitCode).toBe(143);
595596
expect(result.blockedDomains).toEqual([]);
596597
});
598+
599+
it('should recognize domains matched by a wildcard allowlist entry', async () => {
600+
const squidLogsDir = path.join(testDir, 'squid-logs');
601+
fs.mkdirSync(squidLogsDir, { recursive: true });
602+
// api.github.com is blocked on a non-standard port
603+
fs.writeFileSync(
604+
path.join(squidLogsDir, 'access.log'),
605+
'1760994429.358 172.30.0.20:36274 api.github.com:8443 -:- 1.1 CONNECT 403 TCP_DENIED:HIER_NONE api.github.com:8443 "curl/7.81.0"\n'
606+
);
607+
608+
mockExecaFn.mockResolvedValueOnce({ stdout: '', stderr: '', exitCode: 0 } as any); // docker logs -f
609+
mockExecaFn.mockResolvedValueOnce({ stdout: '1', stderr: '', exitCode: 0 } as any); // docker wait
610+
611+
const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
612+
try {
613+
await runAgentCommand(testDir, ['*.github.com']);
614+
// *.github.com covers api.github.com, so the message should report a port issue, not a missing domain
615+
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('port 8443 not allowed'));
616+
expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining('domain not in allowlist'));
617+
} finally {
618+
warnSpy.mockRestore();
619+
}
620+
});
621+
622+
it('should recognize domains matched by a protocol-prefixed allowlist entry', async () => {
623+
const squidLogsDir = path.join(testDir, 'squid-logs');
624+
fs.mkdirSync(squidLogsDir, { recursive: true });
625+
// github.com is listed as https://github.com; a non-standard port block should show as port issue
626+
fs.writeFileSync(
627+
path.join(squidLogsDir, 'access.log'),
628+
'1760994429.358 172.30.0.20:36274 github.com:8080 -:- 1.1 CONNECT 403 TCP_DENIED:HIER_NONE github.com:8080 "curl/7.81.0"\n'
629+
);
630+
631+
mockExecaFn.mockResolvedValueOnce({ stdout: '', stderr: '', exitCode: 0 } as any); // docker logs -f
632+
mockExecaFn.mockResolvedValueOnce({ stdout: '1', stderr: '', exitCode: 0 } as any); // docker wait
633+
634+
const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
635+
try {
636+
await runAgentCommand(testDir, ['https://github.com']);
637+
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('port 8080 not allowed'));
638+
expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining('domain not in allowlist'));
639+
} finally {
640+
warnSpy.mockRestore();
641+
}
642+
});
643+
644+
it('should deduplicate domains in --allow-domains suggestion', async () => {
645+
const squidLogsDir = path.join(testDir, 'squid-logs');
646+
fs.mkdirSync(squidLogsDir, { recursive: true });
647+
// Same domain blocked on two different ports — should appear once in the suggestion
648+
fs.writeFileSync(
649+
path.join(squidLogsDir, 'access.log'),
650+
'1760994429.358 172.30.0.20:36274 missing.com:80 -:- 1.1 GET 403 TCP_DENIED:HIER_NONE missing.com:80 "curl/7.81.0"\n' +
651+
'1760994430.000 172.30.0.20:36275 missing.com:443 -:- 1.1 CONNECT 403 TCP_DENIED:HIER_NONE missing.com:443 "curl/7.81.0"\n'
652+
);
653+
654+
mockExecaFn.mockResolvedValueOnce({ stdout: '', stderr: '', exitCode: 0 } as any); // docker logs -f
655+
mockExecaFn.mockResolvedValueOnce({ stdout: '1', stderr: '', exitCode: 0 } as any); // docker wait
656+
657+
const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
658+
try {
659+
await runAgentCommand(testDir, ['github.com']);
660+
const suggestionCalls = warnSpy.mock.calls.filter(([msg]) =>
661+
typeof msg === 'string' && msg.includes('--allow-domains')
662+
);
663+
expect(suggestionCalls).toHaveLength(1);
664+
const suggestion = suggestionCalls[0][0] as string;
665+
// missing.com should appear exactly once in the suggestion
666+
const occurrences = (suggestion.match(/missing\.com/g) ?? []).length;
667+
expect(occurrences).toBe(1);
668+
} finally {
669+
warnSpy.mockRestore();
670+
}
671+
});
672+
673+
it('should use logger.warn (not logger.error) for post-run blocked-domain diagnostics', async () => {
674+
const squidLogsDir = path.join(testDir, 'squid-logs');
675+
fs.mkdirSync(squidLogsDir, { recursive: true });
676+
fs.writeFileSync(
677+
path.join(squidLogsDir, 'access.log'),
678+
'1760994429.358 172.30.0.20:36274 blocked.com:443 -:- 1.1 CONNECT 403 TCP_DENIED:HIER_NONE blocked.com:443 "curl/7.81.0"\n'
679+
);
680+
681+
mockExecaFn.mockResolvedValueOnce({ stdout: '', stderr: '', exitCode: 0 } as any); // docker logs -f
682+
mockExecaFn.mockResolvedValueOnce({ stdout: '1', stderr: '', exitCode: 0 } as any); // docker wait
683+
684+
const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
685+
const errorSpy = jest.spyOn(logger, 'error').mockImplementation(() => {});
686+
try {
687+
await runAgentCommand(testDir, ['github.com']);
688+
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('blocked.com'));
689+
expect(errorSpy).not.toHaveBeenCalledWith(expect.stringContaining('blocked.com'));
690+
} finally {
691+
warnSpy.mockRestore();
692+
errorSpy.mockRestore();
693+
}
694+
});
597695
});
598696
});

0 commit comments

Comments
 (0)