fix: extract api-target logic into testable helper, add unit tests

src/cli.test.ts

@@ -1,5 +1,5 @@
 import { Command } from 'commander';
-import { parseEnvironmentVariables, parseDomains, parseDomainsFile, escapeShellArg, joinShellArgs, parseVolumeMounts, isValidIPv4, isValidIPv6, parseDnsServers, parseDnsOverHttps, validateAgentImage, isAgentImagePreset, AGENT_IMAGE_PRESETS, processAgentImageOption, processLocalhostKeyword, validateSkipPullWithBuildLocal, validateAllowHostPorts, parseMemoryLimit, validateFormat, validateApiProxyConfig, buildRateLimitConfig, validateRateLimitFlags, hasRateLimitOptions, collectRulesetFile, validateApiTargetInAllowedDomains, DEFAULT_OPENAI_API_TARGET, DEFAULT_ANTHROPIC_API_TARGET, DEFAULT_COPILOT_API_TARGET, emitApiProxyTargetWarnings, formatItem, program, parseAgentTimeout, applyAgentTimeout, handlePredownloadAction } from './cli';
+import { parseEnvironmentVariables, parseDomains, parseDomainsFile, escapeShellArg, joinShellArgs, parseVolumeMounts, isValidIPv4, isValidIPv6, parseDnsServers, parseDnsOverHttps, validateAgentImage, isAgentImagePreset, AGENT_IMAGE_PRESETS, processAgentImageOption, processLocalhostKeyword, validateSkipPullWithBuildLocal, validateAllowHostPorts, parseMemoryLimit, validateFormat, validateApiProxyConfig, buildRateLimitConfig, validateRateLimitFlags, hasRateLimitOptions, collectRulesetFile, validateApiTargetInAllowedDomains, DEFAULT_OPENAI_API_TARGET, DEFAULT_ANTHROPIC_API_TARGET, DEFAULT_COPILOT_API_TARGET, emitApiProxyTargetWarnings, formatItem, program, parseAgentTimeout, applyAgentTimeout, handlePredownloadAction, resolveApiTargetsToAllowedDomains } from './cli';
 import { redactSecrets } from './redact-secrets';
 import * as fs from 'fs';
 import * as path from 'path';
@@ -1852,6 +1852,135 @@ describe('cli', () => {
     });
   });
 
+  describe('resolveApiTargetsToAllowedDomains', () => {
+    it('should add copilot-api-target option to allowed domains', () => {
+      const domains: string[] = ['github.com'];
+      resolveApiTargetsToAllowedDomains({ copilotApiTarget: 'custom.copilot.com' }, domains);
+      expect(domains).toContain('custom.copilot.com');
+      expect(domains).toContain('https://custom.copilot.com');
+    });
+
+    it('should add openai-api-target option to allowed domains', () => {
+      const domains: string[] = ['github.com'];
+      resolveApiTargetsToAllowedDomains({ openaiApiTarget: 'custom.openai.com' }, domains);
+      expect(domains).toContain('custom.openai.com');
+      expect(domains).toContain('https://custom.openai.com');
+    });
+
+    it('should add anthropic-api-target option to allowed domains', () => {
+      const domains: string[] = ['github.com'];
+      resolveApiTargetsToAllowedDomains({ anthropicApiTarget: 'custom.anthropic.com' }, domains);
+      expect(domains).toContain('custom.anthropic.com');
+      expect(domains).toContain('https://custom.anthropic.com');
+    });
+
+    it('should prefer option flag over env var', () => {
+      const domains: string[] = [];
+      const env = { COPILOT_API_TARGET: 'env.copilot.com' };
+      resolveApiTargetsToAllowedDomains({ copilotApiTarget: 'flag.copilot.com' }, domains, env);
+      expect(domains).toContain('flag.copilot.com');
+      expect(domains).not.toContain('env.copilot.com');
+    });
+
+    it('should fall back to env var when option flag is not set', () => {
+      const domains: string[] = [];
+      const env = { COPILOT_API_TARGET: 'env.copilot.com' };
+      resolveApiTargetsToAllowedDomains({}, domains, env);
+      expect(domains).toContain('env.copilot.com');
+      expect(domains).toContain('https://env.copilot.com');
+    });
+
+    it('should read OPENAI_API_TARGET from env when flag not set', () => {
+      const domains: string[] = [];
+      const env = { OPENAI_API_TARGET: 'env.openai.com' };
+      resolveApiTargetsToAllowedDomains({}, domains, env);
+      expect(domains).toContain('env.openai.com');
+    });
+
+    it('should read ANTHROPIC_API_TARGET from env when flag not set', () => {
+      const domains: string[] = [];
+      const env = { ANTHROPIC_API_TARGET: 'env.anthropic.com' };
+      resolveApiTargetsToAllowedDomains({}, domains, env);
+      expect(domains).toContain('env.anthropic.com');
+    });
+
+    it('should not duplicate a domain already in the list', () => {
+      const domains: string[] = ['custom.copilot.com'];
+      resolveApiTargetsToAllowedDomains({ copilotApiTarget: 'custom.copilot.com' }, domains);
+      const count = domains.filter(d => d === 'custom.copilot.com').length;
+      expect(count).toBe(1);
+    });
+
+    it('should not duplicate the https:// form if already in the list', () => {
+      const domains: string[] = ['github.com', 'https://custom.copilot.com'];
+      resolveApiTargetsToAllowedDomains({ copilotApiTarget: 'custom.copilot.com' }, domains);
+      const count = domains.filter(d => d === 'https://custom.copilot.com').length;
+      expect(count).toBe(1);
+    });
+
+    it('should preserve an existing https:// prefix without doubling it', () => {
+      const domains: string[] = [];
+      resolveApiTargetsToAllowedDomains({ copilotApiTarget: 'https://custom.copilot.com' }, domains);
+      expect(domains).toContain('https://custom.copilot.com');
+      const count = domains.filter(d => d === 'https://custom.copilot.com').length;
+      expect(count).toBe(1);
+    });
+
+    it('should handle http:// prefix without adding another https://', () => {
+      const domains: string[] = [];
+      resolveApiTargetsToAllowedDomains({ openaiApiTarget: 'http://internal.openai.com' }, domains);
+      expect(domains).toContain('http://internal.openai.com');
+    });
+
+    it('should add all three targets when all are specified', () => {
+      const domains: string[] = [];
+      resolveApiTargetsToAllowedDomains(
+        {
+          copilotApiTarget: 'copilot.internal',
+          openaiApiTarget: 'openai.internal',
+          anthropicApiTarget: 'anthropic.internal',
+        },
+        domains
+      );
+      expect(domains).toContain('copilot.internal');
+      expect(domains).toContain('openai.internal');
+      expect(domains).toContain('anthropic.internal');
+    });
+
+    it('should call debug with auto-added domains', () => {
+      const domains: string[] = [];
+      const debugMessages: string[] = [];
+      resolveApiTargetsToAllowedDomains(
+        { copilotApiTarget: 'copilot.internal' },
+        domains,
+        {},
+        (msg) => debugMessages.push(msg)
+      );
+      expect(debugMessages.some(m => m.includes('copilot.internal'))).toBe(true);
+    });
+
+    it('should not call debug when no api targets are set', () => {
+      const domains: string[] = [];
+      const debugMessages: string[] = [];
+      resolveApiTargetsToAllowedDomains({}, domains, {}, (msg) => debugMessages.push(msg));
+      expect(debugMessages).toHaveLength(0);
+    });
+
+    it('should return the same allowedDomains array reference', () => {
+      const domains: string[] = [];
+      const returned = resolveApiTargetsToAllowedDomains({ copilotApiTarget: 'x.com' }, domains);
+      expect(returned).toBe(domains);
+    });
+
+    it('should ignore empty env var values', () => {
+      const domains: string[] = [];
+      const env = { COPILOT_API_TARGET: '   ', OPENAI_API_TARGET: '' };
+      resolveApiTargetsToAllowedDomains({}, domains, env);
+      // Whitespace-only and empty values are filtered out
+      expect(domains).toHaveLength(0);
+    });
+  });
+
   describe('formatItem', () => {
     it('should format item with description on same line when term fits', () => {
       const result = formatItem('-v', 'verbose output', 20, 2, 2, 80);

src/cli.ts

@@ -378,6 +378,72 @@ export function emitApiProxyTargetWarnings(
   }
 }
 
+/**
+ * Resolves API target values from CLI options and environment variables, and merges them
+ * into the allowed domains list. Also ensures each target is present as an https:// URL.
+ * @param options - Partial options with API target flag values
+ * @param allowedDomains - The current list of allowed domains (mutated in place)
+ * @param env - Environment variables (defaults to process.env)
+ * @param debug - Optional debug logging function
+ * @returns The updated allowedDomains array (same reference, mutated)
+ */
+export function resolveApiTargetsToAllowedDomains(
+  options: {
+    copilotApiTarget?: string;
+    openaiApiTarget?: string;
+    anthropicApiTarget?: string;
+  },
+  allowedDomains: string[],
+  env: Record<string, string | undefined> = process.env,
+  debug: (msg: string) => void = () => {}
+): string[] {
+  const apiTargets: string[] = [];
+
+  if (options.copilotApiTarget) {
+    apiTargets.push(options.copilotApiTarget);
+  } else if (env['COPILOT_API_TARGET']) {
+    apiTargets.push(env['COPILOT_API_TARGET']);
+  }
+
+  if (options.openaiApiTarget) {
+    apiTargets.push(options.openaiApiTarget);
+  } else if (env['OPENAI_API_TARGET']) {
+    apiTargets.push(env['OPENAI_API_TARGET']);
+  }
+
+  if (options.anthropicApiTarget) {
+    apiTargets.push(options.anthropicApiTarget);
+  } else if (env['ANTHROPIC_API_TARGET']) {
+    apiTargets.push(env['ANTHROPIC_API_TARGET']);
+  }
+
+  // Merge raw target values into the allowedDomains list so that later
+  // checks/logs about "no allowed domains" see the final, expanded allowlist.
+  const normalizedApiTargets = apiTargets.filter((t) => typeof t === 'string' && t.trim().length > 0);
+  if (normalizedApiTargets.length > 0) {
+    for (const target of normalizedApiTargets) {
+      if (!allowedDomains.includes(target)) {
+        allowedDomains.push(target);
+      }
+    }
+    debug(`Auto-added API target values to allowed domains: ${normalizedApiTargets.join(', ')}`);
+  }
+
+  // Also ensure each target is present as an explicit https:// URL
+  for (const target of normalizedApiTargets) {
+
+    // Ensure auto-added API targets are explicitly HTTPS to avoid over-broad HTTP+HTTPS allowlisting
+    const normalizedTarget = /^https?:\/\//.test(target) ? target : `https://${target}`;
+
+    if (!allowedDomains.includes(normalizedTarget)) {
+      allowedDomains.push(normalizedTarget);
+      debug(`Automatically added API target to allowlist: ${normalizedTarget}`);
+    }
+  }
+
+  return allowedDomains;
+}
+
 /**
  * Builds a RateLimitConfig from parsed CLI options.
  */
@@ -1239,52 +1305,7 @@ program
     // Automatically add API target values to allowlist when specified
     // This ensures that when engine.api-target is set in GitHub Agentic Workflows,
     // the target domain is automatically accessible through the firewall
-    const apiTargets: string[] = [];
-
-    if (options.copilotApiTarget) {
-      apiTargets.push(options.copilotApiTarget);
-    } else if (process.env.COPILOT_API_TARGET) {
-      apiTargets.push(process.env.COPILOT_API_TARGET);
-    }
-
-    if (options.openaiApiTarget) {
-      apiTargets.push(options.openaiApiTarget);
-    } else if (process.env.OPENAI_API_TARGET) {
-      apiTargets.push(process.env.OPENAI_API_TARGET);
-    }
-
-    if (options.anthropicApiTarget) {
-      apiTargets.push(options.anthropicApiTarget);
-    } else if (process.env.ANTHROPIC_API_TARGET) {
-      apiTargets.push(process.env.ANTHROPIC_API_TARGET);
-    }
-
-    // Merge any API target values into the allowedDomains list so that later
-    // checks/logs about "no allowed domains" see the final, expanded allowlist.
-    const normalizedApiTargets = apiTargets.filter((t) => typeof t === 'string' && t.trim().length > 0);
-    if (normalizedApiTargets.length > 0) {
-      for (const target of normalizedApiTargets) {
-        if (!allowedDomains.includes(target)) {
-          allowedDomains.push(target);
-        }
-      }
-      logger.debug(`Auto-added API target values to allowed domains: ${normalizedApiTargets.join(', ')}`);
-    }
-
-    // Add API targets to allowedDomains if not already present
-    for (const target of apiTargets) {
-      if (!target) {
-        continue;
-      }
-
-      // Ensure auto-added API targets are explicitly HTTPS to avoid over-broad HTTP+HTTPS allowlisting
-      const normalizedTarget = /^https?:\/\//.test(target) ? target : `https://${target}`;
-
-      if (!allowedDomains.includes(normalizedTarget)) {
-        allowedDomains.push(normalizedTarget);
-        logger.debug(`Automatically added API target to allowlist: ${normalizedTarget}`);
-      }
-    }
+    resolveApiTargetsToAllowedDomains(options, allowedDomains, process.env, logger.debug.bind(logger));
 
     // Validate all domains and patterns
     for (const domain of allowedDomains) {