Skip to content

fix(firewall): add api-proxy to allowed domains when enabled #804

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 2 commits into from
Feb 13, 2026
Merged

fix(firewall): add api-proxy to allowed domains when enabled #804

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
df73ab4
Initial plan
Claude Feb 13, 2026
a784a8d
fix(firewall): add api-proxy to allowed domains when enabled
Claude Feb 13, 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
51 changes: 51 additions & 0 deletions src/docker-manager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1854,6 +1854,57 @@ describe('docker-manager', () => {
process.env.SUDO_USER = originalSudoUser;
}
});

it('should include api-proxy in allowed domains when enableApiProxy is true', async () => {
const config: WrapperConfig = {
allowedDomains: ['github.com'],
agentCommand: 'echo test',
logLevel: 'info',
keepContainers: false,
workDir: testDir,
enableApiProxy: true,
openaiApiKey: 'sk-test-key',
};

try {
await writeConfigs(config);
} catch {
// May fail after writing configs
}

// Verify squid.conf includes api-proxy in allowed domains
const squidConfPath = path.join(testDir, 'squid.conf');
if (fs.existsSync(squidConfPath)) {
const content = fs.readFileSync(squidConfPath, 'utf-8');
expect(content).toContain('github.com');
expect(content).toContain('api-proxy');
}
Comment on lines +1869 to +1881

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

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

This test can pass without actually asserting anything: it swallows writeConfigs errors and then only checks the file contents if squid.conf exists. If writeConfigs fails before writing the config (e.g., missing seccomp profile), the if (fs.existsSync(...)) block is skipped and the test still passes. To make the coverage meaningful, ensure squid.conf is created (or mock the seccomp-profile copy) and assert existsSync is true, or spy on generateSquidConfig to verify the domains argument includes api-proxy.

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

Copilot uses AI. Check for mistakes.
});

it('should not include api-proxy in allowed domains when enableApiProxy is false', async () => {
const config: WrapperConfig = {
allowedDomains: ['github.com'],
agentCommand: 'echo test',
logLevel: 'info',
keepContainers: false,
workDir: testDir,
enableApiProxy: false,
};

try {
await writeConfigs(config);
} catch {
// May fail after writing configs
}

// Verify squid.conf does not include api-proxy when disabled
const squidConfPath = path.join(testDir, 'squid.conf');
if (fs.existsSync(squidConfPath)) {
const content = fs.readFileSync(squidConfPath, 'utf-8');
expect(content).toContain('github.com');
expect(content).not.toContain('api-proxy');
}
});
});

describe('startContainers', () => {
Expand Down
7 changes: 6 additions & 1 deletion src/docker-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1112,8 +1112,13 @@ export async function writeConfigs(config: WrapperConfig): Promise<void> {

// Write Squid config
// Note: Use container path for SSL database since it's mounted at /var/spool/squid_ssl_db
// When API proxy is enabled, add api-proxy to allowed domains so agent can communicate with it
const domainsForSquid = config.enableApiProxy && networkConfig.proxyIp
? [...config.allowedDomains, 'api-proxy']
Comment on lines +1115 to +1117

Copilot AI Feb 13, 2026

Copy link

Choose a reason for hiding this comment

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

domainsForSquid appends 'api-proxy' unconditionally when enabled, which can create duplicate entries if the user already included api-proxy (or .api-proxy) in allowedDomains. Consider de-duping (e.g., via a Set or an includes check) before passing the list into generateSquidConfig to avoid redundant ACL lines.

Suggested change
// When API proxy is enabled, add api-proxy to allowed domains so agent can communicate with it
const domainsForSquid = config.enableApiProxy && networkConfig.proxyIp
? [...config.allowedDomains, 'api-proxy']
// Note: Use container path for SSL database since it's mounted at /var/spool/squid_ssl_db
// When API proxy is enabled, add api-proxy to allowed domains so agent can communicate with it,
// but avoid duplicating entries if the user already specified api-proxy (or .api-proxy).
const hasApiProxyDomain =
config.allowedDomains.includes('api-proxy') ||
config.allowedDomains.includes('.api-proxy');
const domainsForSquid = config.enableApiProxy && networkConfig.proxyIp
? (hasApiProxyDomain ? config.allowedDomains : [...config.allowedDomains, 'api-proxy'])

Copilot uses AI. Check for mistakes.
: config.allowedDomains;

const squidConfig = generateSquidConfig({
domains: config.allowedDomains,
domains: domainsForSquid,
blockedDomains: config.blockedDomains,
port: SQUID_PORT,
sslBump: config.sslBump,
Expand Down
Loading