Skip to content

Commit 07f9438

Browse files
lpcoxCopilotCopilot
authored
fix: normalize API target env vars to bare hostnames via URL parsing (#1799)
* fix: normalize API target env vars to strip scheme prefix When --*-api-target values originate from GitHub Actions expressions (e.g., ${{ vars.ANTHROPIC_BASE_URL }}), the scheme is not stripped at compile time. At runtime the full URL (https://host) reaches the API proxy, which prepends https:// again, producing double-scheme URLs like https://https://host that Squid rejects. Fix at two layers: 1. containers/api-proxy/server.js: add normalizeApiTarget() that strips any http(s):// prefix. Applied to all four API targets (OpenAI, Anthropic, Gemini, Copilot) on startup. 2. src/docker-manager.ts: add stripScheme() and apply it when setting *_API_TARGET env vars in the api-proxy container. This prevents the scheme from ever reaching the container. Fixes #1795 Upstream: github/gh-aw#25137 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: normalize API targets to bare hostnames only Use URL parsing in both normalizeApiTarget() and stripScheme() to extract only the hostname, discarding path, query, fragment, port, and credentials. Paths must use the separate *_API_BASE_PATH settings. Addresses review feedback that target values are used as TLS SNI / hostname in https.request(), so including paths would break requests. Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/ab65b061-c924-41d8-a573-dd954497def2 * fix: improve error message in normalizeApiTarget Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/ab65b061-c924-41d8-a573-dd954497def2 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
1 parent 8956001 commit 07f9438

File tree

4 files changed

+171
-11
lines changed

4 files changed

+171
-11
lines changed

containers/api-proxy/server.js

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,48 @@ const ANTHROPIC_API_KEY = (process.env.ANTHROPIC_API_KEY || '').trim() || undefi
6464
const COPILOT_GITHUB_TOKEN = (process.env.COPILOT_GITHUB_TOKEN || '').trim() || undefined;
6565
const GEMINI_API_KEY = (process.env.GEMINI_API_KEY || '').trim() || undefined;
6666

67+
/**
68+
* Normalizes an API target value to a bare hostname.
69+
* Accepts either a hostname or a full URL and extracts only the hostname,
70+
* discarding any scheme, path, query, fragment, credentials, or port.
71+
* Path configuration must be provided separately via the existing
72+
* *_API_BASE_PATH environment variables.
73+
*
74+
* @param {string|undefined} value - Raw env var value
75+
* @returns {string|undefined} Bare hostname, or undefined if input is falsy
76+
*/
77+
function normalizeApiTarget(value) {
78+
if (!value) return value;
79+
80+
const trimmed = value.trim();
81+
if (!trimmed) return undefined;
82+
83+
const candidate = /^[a-zA-Z][a-zA-Z\d+\-.]*:\/\//.test(trimmed)
84+
? trimmed
85+
: `https://${trimmed}`;
86+
87+
try {
88+
const parsed = new URL(candidate);
89+
90+
if (parsed.pathname !== '/' || parsed.search || parsed.hash || parsed.username || parsed.password || parsed.port) {
91+
console.warn(
92+
`Ignoring unsupported API target URL components in ${sanitizeForLog(trimmed)}; ` +
93+
'configure path prefixes via the corresponding *_API_BASE_PATH environment variable.'
94+
);
95+
}
96+
97+
return parsed.hostname || undefined;
98+
} catch (err) {
99+
console.warn(`Invalid API target ${sanitizeForLog(trimmed)}; expected a hostname (e.g. 'api.example.com') or URL`);
100+
return undefined;
101+
}
102+
}
103+
67104
// Configurable API target hosts (supports custom endpoints / internal LLM routers)
68-
const OPENAI_API_TARGET = process.env.OPENAI_API_TARGET || 'api.openai.com';
69-
const ANTHROPIC_API_TARGET = process.env.ANTHROPIC_API_TARGET || 'api.anthropic.com';
70-
const GEMINI_API_TARGET = process.env.GEMINI_API_TARGET || 'generativelanguage.googleapis.com';
105+
// Values are normalized to bare hostnames — buildUpstreamPath() prepends https://
106+
const OPENAI_API_TARGET = normalizeApiTarget(process.env.OPENAI_API_TARGET) || 'api.openai.com';
107+
const ANTHROPIC_API_TARGET = normalizeApiTarget(process.env.ANTHROPIC_API_TARGET) || 'api.anthropic.com';
108+
const GEMINI_API_TARGET = normalizeApiTarget(process.env.GEMINI_API_TARGET) || 'generativelanguage.googleapis.com';
71109

72110
/**
73111
* Normalizes a base path for use as a URL path prefix.
@@ -123,7 +161,7 @@ const GEMINI_API_BASE_PATH = normalizeBasePath(process.env.GEMINI_API_BASE_PATH)
123161
// Priority: COPILOT_API_TARGET env var > auto-derive from GITHUB_SERVER_URL > default
124162
function deriveCopilotApiTarget() {
125163
if (process.env.COPILOT_API_TARGET) {
126-
return process.env.COPILOT_API_TARGET;
164+
return normalizeApiTarget(process.env.COPILOT_API_TARGET);
127165
}
128166
// Auto-derive from GITHUB_SERVER_URL:
129167
// - GitHub Enterprise Cloud (*.ghe.com): Copilot inference/models/MCP are served at
@@ -932,4 +970,4 @@ if (require.main === module) {
932970
}
933971

934972
// Export for testing
935-
module.exports = { deriveCopilotApiTarget, normalizeBasePath, buildUpstreamPath, proxyWebSocket };
973+
module.exports = { normalizeApiTarget, deriveCopilotApiTarget, normalizeBasePath, buildUpstreamPath, proxyWebSocket };

containers/api-proxy/server.test.js

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,46 @@
55
const http = require('http');
66
const tls = require('tls');
77
const { EventEmitter } = require('events');
8-
const { deriveCopilotApiTarget, normalizeBasePath, buildUpstreamPath, proxyWebSocket } = require('./server');
8+
const { normalizeApiTarget, deriveCopilotApiTarget, normalizeBasePath, buildUpstreamPath, proxyWebSocket } = require('./server');
9+
10+
describe('normalizeApiTarget', () => {
11+
it('should strip https:// prefix', () => {
12+
expect(normalizeApiTarget('https://my-gateway.example.com')).toBe('my-gateway.example.com');
13+
});
14+
15+
it('should strip http:// prefix', () => {
16+
expect(normalizeApiTarget('http://my-gateway.example.com')).toBe('my-gateway.example.com');
17+
});
18+
19+
it('should preserve bare hostname', () => {
20+
expect(normalizeApiTarget('api.openai.com')).toBe('api.openai.com');
21+
});
22+
23+
it('should normalize a URL with a path to just the hostname', () => {
24+
expect(normalizeApiTarget('https://my-gateway.example.com/some-path')).toBe('my-gateway.example.com');
25+
});
26+
27+
it('should trim whitespace', () => {
28+
expect(normalizeApiTarget(' https://api.openai.com ')).toBe('api.openai.com');
29+
});
30+
31+
it('should return undefined for falsy input', () => {
32+
expect(normalizeApiTarget(undefined)).toBeUndefined();
33+
expect(normalizeApiTarget('')).toBe('');
34+
});
35+
36+
it('should not strip scheme-like substrings in the middle', () => {
37+
expect(normalizeApiTarget('api.https.example.com')).toBe('api.https.example.com');
38+
});
39+
40+
it('should discard port from URL', () => {
41+
expect(normalizeApiTarget('https://my-gateway.example.com:8443')).toBe('my-gateway.example.com');
42+
});
43+
44+
it('should discard query and fragment from URL', () => {
45+
expect(normalizeApiTarget('https://my-gateway.example.com/path?key=val#frag')).toBe('my-gateway.example.com');
46+
});
47+
});
948

1049
describe('deriveCopilotApiTarget', () => {
1150
let originalEnv;
@@ -268,6 +307,25 @@ describe('buildUpstreamPath', () => {
268307
.toBe('/serving-endpoints/v1/chat/completions');
269308
});
270309
});
310+
311+
describe('with normalized API target (gh-aw#25137 regression)', () => {
312+
it('should produce correct path when target was already normalized', () => {
313+
// normalizeApiTarget('https://my-gateway.example.com/some-path')
314+
// returns 'my-gateway.example.com' (hostname only)
315+
const target = 'my-gateway.example.com';
316+
expect(buildUpstreamPath('/v1/messages', target, ''))
317+
.toBe('/v1/messages');
318+
});
319+
320+
it('should produce wrong hostname if scheme is NOT stripped (demonstrating the bug)', () => {
321+
// Without normalizeApiTarget, the scheme-prefixed value causes
322+
// new URL() to parse 'https' as the hostname instead of the real host
323+
const badTarget = 'https://my-gateway.example.com';
324+
const targetUrl = new URL('/v1/messages', `https://${badTarget}`);
325+
// Node parses this as hostname='https', not 'my-gateway.example.com'
326+
expect(targetUrl.hostname).not.toBe('my-gateway.example.com');
327+
});
328+
});
271329
});
272330

273331
// ── Helpers for proxyWebSocket tests ──────────────────────────────────────────

src/docker-manager.test.ts

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { generateDockerCompose, subnetsOverlap, writeConfigs, startContainers, stopContainers, fastKillAgentContainer, isAgentExternallyKilled, resetAgentExternallyKilled, AGENT_CONTAINER_NAME, cleanup, runAgentCommand, validateIdNotInSystemRange, getSafeHostUid, getSafeHostGid, getRealUserHome, extractGhHostFromServerUrl, readGitHubPathEntries, mergeGitHubPathEntries, readEnvFile, MIN_REGULAR_UID, ACT_PRESET_BASE_IMAGE } from './docker-manager';
1+
import { generateDockerCompose, subnetsOverlap, writeConfigs, startContainers, stopContainers, fastKillAgentContainer, isAgentExternallyKilled, resetAgentExternallyKilled, AGENT_CONTAINER_NAME, cleanup, runAgentCommand, validateIdNotInSystemRange, getSafeHostUid, getSafeHostGid, getRealUserHome, extractGhHostFromServerUrl, readGitHubPathEntries, mergeGitHubPathEntries, readEnvFile, MIN_REGULAR_UID, ACT_PRESET_BASE_IMAGE, stripScheme } from './docker-manager';
22
import { WrapperConfig } from './types';
33
import * as fs from 'fs';
44
import * as path from 'path';
@@ -295,6 +295,28 @@ describe('docker-manager', () => {
295295
});
296296
});
297297

298+
describe('stripScheme', () => {
299+
it('should strip https:// prefix', () => {
300+
expect(stripScheme('https://my-gateway.example.com')).toBe('my-gateway.example.com');
301+
});
302+
303+
it('should strip http:// prefix', () => {
304+
expect(stripScheme('http://my-gateway.example.com')).toBe('my-gateway.example.com');
305+
});
306+
307+
it('should preserve bare hostname', () => {
308+
expect(stripScheme('api.openai.com')).toBe('api.openai.com');
309+
});
310+
311+
it('should normalize URL with path to hostname only', () => {
312+
expect(stripScheme('https://my-gateway.example.com/some-path')).toBe('my-gateway.example.com');
313+
});
314+
315+
it('should not strip scheme-like substrings in the middle', () => {
316+
expect(stripScheme('api.https.example.com')).toBe('api.https.example.com');
317+
});
318+
});
319+
298320
describe('generateDockerCompose', () => {
299321
const mockConfig: WrapperConfig = {
300322
allowedDomains: ['github.com', 'npmjs.org'],
@@ -2439,6 +2461,22 @@ describe('docker-manager', () => {
24392461
expect(env.ANTHROPIC_API_TARGET).toBe('custom.anthropic-router.internal');
24402462
});
24412463

2464+
it('should strip https:// scheme from API target values (gh-aw#25137)', () => {
2465+
const configWithProxy = {
2466+
...mockConfig,
2467+
enableApiProxy: true,
2468+
anthropicApiKey: 'sk-ant-test-key',
2469+
anthropicApiTarget: 'https://my-gateway.example.com',
2470+
openaiApiKey: 'sk-openai-test',
2471+
openaiApiTarget: 'https://openai-router.internal',
2472+
};
2473+
const result = generateDockerCompose(configWithProxy, mockNetworkConfigWithProxy);
2474+
const proxy = result.services['api-proxy'];
2475+
const env = proxy.environment as Record<string, string>;
2476+
expect(env.ANTHROPIC_API_TARGET).toBe('my-gateway.example.com');
2477+
expect(env.OPENAI_API_TARGET).toBe('openai-router.internal');
2478+
});
2479+
24422480
it('should not set ANTHROPIC_API_TARGET in api-proxy when anthropicApiTarget is not provided', () => {
24432481
const configWithProxy = { ...mockConfig, enableApiProxy: true, anthropicApiKey: 'sk-ant-test-key' };
24442482
const result = generateDockerCompose(configWithProxy, mockNetworkConfigWithProxy);

src/docker-manager.ts

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,29 @@ export interface SslConfig {
372372
sslDbPath: string;
373373
}
374374

375+
/**
376+
* Normalizes an API target value to a bare hostname.
377+
* API target values should be bare hostnames (e.g., "api.openai.com"), but
378+
* may arrive with a scheme or path when set via GitHub Actions expressions
379+
* that are resolved at runtime (see github/gh-aw#25137).
380+
* Discards any scheme, path, query, fragment, credentials, or port —
381+
* path prefixes must use the separate *_API_BASE_PATH settings.
382+
*/
383+
export function stripScheme(value: string): string {
384+
const trimmed = value.trim();
385+
if (!trimmed) return trimmed;
386+
387+
const candidate = /^[a-zA-Z][a-zA-Z\d+\-.]*:\/\//.test(trimmed)
388+
? trimmed
389+
: `https://${trimmed}`;
390+
391+
try {
392+
return new URL(candidate).hostname || trimmed;
393+
} catch {
394+
return trimmed;
395+
}
396+
}
397+
375398
/**
376399
* Generates Docker Compose configuration
377400
* Note: Uses external network 'awf-net' created by host-iptables setup
@@ -1460,12 +1483,15 @@ export function generateDockerCompose(
14601483
...(config.copilotGithubToken && { COPILOT_GITHUB_TOKEN: config.copilotGithubToken }),
14611484
...(config.geminiApiKey && { GEMINI_API_KEY: config.geminiApiKey }),
14621485
// Configurable API targets (for GHES/GHEC / custom endpoints)
1463-
...(config.copilotApiTarget && { COPILOT_API_TARGET: config.copilotApiTarget }),
1464-
...(config.openaiApiTarget && { OPENAI_API_TARGET: config.openaiApiTarget }),
1486+
// Strip any scheme prefix — server.js also normalizes defensively, but
1487+
// stripping here prevents a scheme-prefixed hostname from reaching the
1488+
// container at all (belt-and-suspenders for gh-aw#25137).
1489+
...(config.copilotApiTarget && { COPILOT_API_TARGET: stripScheme(config.copilotApiTarget) }),
1490+
...(config.openaiApiTarget && { OPENAI_API_TARGET: stripScheme(config.openaiApiTarget) }),
14651491
...(config.openaiApiBasePath && { OPENAI_API_BASE_PATH: config.openaiApiBasePath }),
1466-
...(config.anthropicApiTarget && { ANTHROPIC_API_TARGET: config.anthropicApiTarget }),
1492+
...(config.anthropicApiTarget && { ANTHROPIC_API_TARGET: stripScheme(config.anthropicApiTarget) }),
14671493
...(config.anthropicApiBasePath && { ANTHROPIC_API_BASE_PATH: config.anthropicApiBasePath }),
1468-
...(config.geminiApiTarget && { GEMINI_API_TARGET: config.geminiApiTarget }),
1494+
...(config.geminiApiTarget && { GEMINI_API_TARGET: stripScheme(config.geminiApiTarget) }),
14691495
...(config.geminiApiBasePath && { GEMINI_API_BASE_PATH: config.geminiApiBasePath }),
14701496
// Forward GITHUB_SERVER_URL so api-proxy can auto-derive enterprise endpoints
14711497
...(process.env.GITHUB_SERVER_URL && { GITHUB_SERVER_URL: process.env.GITHUB_SERVER_URL }),

0 commit comments

Comments
 (0)