Skip to content

Commit 1cbfa3a

Browse files
authored
fix(api-proxy): address code review feedback
- proxy-utils.js: sanitize raw target in warn logs (log injection) - copilot.js: fallback to default when COPILOT_API_TARGET is malformed - openai/anthropic/gemini: getModelsFetchConfig respects *_API_BASE_PATH - anthropic.js: cache composedBodyTransform at startup (not per-request) - ADDING-A-PROVIDER.md: fix Dockerfile COPY snippet path - server.test.js: add createProviderServer() tests (health, stubs, transforms, auth) Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/5fe25196-13b3-4865-b327-6d74594f980d
1 parent 9bc4699 commit 1cbfa3a

7 files changed

Lines changed: 266 additions & 12 deletions

File tree

containers/api-proxy/providers/ADDING-A-PROVIDER.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -177,8 +177,8 @@ Add the new adapter file to the explicit `COPY` list in `containers/api-proxy/Do
177177

178178
```dockerfile
179179
COPY server.js logging.js metrics.js rate-limiter.js token-tracker.js \
180-
model-resolver.js proxy-utils.js anthropic-cache.js anthropic-transforms.js \
181-
providers/ ./
180+
model-resolver.js proxy-utils.js anthropic-cache.js anthropic-transforms.js ./
181+
COPY providers/ ./providers/
182182
```
183183

184184
Also update the `EXPOSE` directive to include the new port:

containers/api-proxy/providers/anthropic.js

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,10 @@ function createAnthropicAdapter(env, deps = {}) {
6262

6363
const bodyTransform = deps.bodyTransform || null;
6464

65+
// Build the composed transform once at construction time to avoid
66+
// re-allocating the wrapper function on every request.
67+
const composedBodyTransform = composeBodyTransforms(bodyTransform, optimisationsTransform);
68+
6569
return {
6670
name: 'anthropic',
6771
port: 10001,
@@ -104,9 +108,7 @@ function createAnthropicAdapter(env, deps = {}) {
104108
return headers;
105109
},
106110

107-
getBodyTransform() {
108-
return composeBodyTransforms(bodyTransform, optimisationsTransform);
109-
},
111+
getBodyTransform() { return composedBodyTransform; },
110112

111113
getValidationProbe() {
112114
if (!apiKey) return null;
@@ -130,8 +132,11 @@ function createAnthropicAdapter(env, deps = {}) {
130132

131133
getModelsFetchConfig() {
132134
if (!apiKey) return null;
135+
// Use the configured base path so Anthropic-compatible endpoints with a
136+
// path prefix populate /reflect and models.json correctly.
137+
const modelsPath = basePath ? `${basePath}/models` : '/v1/models';
133138
return {
134-
url: `https://${rawTarget}/v1/models`,
139+
url: `https://${rawTarget}${modelsPath}`,
135140
opts: {
136141
method: 'GET',
137142
headers: { 'x-api-key': apiKey, 'anthropic-version': '2023-06-01' },

containers/api-proxy/providers/copilot.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,10 @@ function resolveCopilotAuthToken(env = process.env) {
4545
*/
4646
function deriveCopilotApiTarget(env = process.env) {
4747
if (env.COPILOT_API_TARGET) {
48-
return normalizeApiTarget(env.COPILOT_API_TARGET);
48+
const target = normalizeApiTarget(env.COPILOT_API_TARGET);
49+
// Only use the explicit value if it parsed into a valid hostname;
50+
// fall through to auto-derivation when the value is malformed.
51+
if (target) return target;
4952
}
5053
const serverUrl = env.GITHUB_SERVER_URL;
5154
if (serverUrl) {

containers/api-proxy/providers/gemini.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,11 @@ function createGeminiAdapter(env, deps = {}) {
8181

8282
getModelsFetchConfig() {
8383
if (!apiKey) return null;
84+
// Use the configured base path so Gemini-compatible endpoints with a
85+
// path prefix populate /reflect and models.json correctly.
86+
const modelsPath = basePath ? `${basePath}/models` : '/v1beta/models';
8487
return {
85-
url: `https://${rawTarget}/v1beta/models`,
88+
url: `https://${rawTarget}${modelsPath}`,
8689
opts: { method: 'GET', headers: { 'x-goog-api-key': apiKey } },
8790
cacheKey: 'gemini',
8891
};

containers/api-proxy/providers/openai.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,13 +76,16 @@ function createOpenAIAdapter(env, deps = {}) {
7676

7777
/**
7878
* Returns the model-list fetch config for /reflect model population, or null.
79+
* Uses the configured base path so prefixed OpenAI-compatible deployments
80+
* (e.g. Databricks, Azure) populate /reflect and models.json correctly.
7981
*
8082
* @returns {{ url: string, opts: object, cacheKey: string }|null}
8183
*/
8284
getModelsFetchConfig() {
8385
if (!apiKey) return null;
86+
const modelsPath = basePath ? `${basePath}/models` : '/v1/models';
8487
return {
85-
url: `https://${rawTarget}/v1/models`,
88+
url: `https://${rawTarget}${modelsPath}`,
8689
opts: { method: 'GET', headers: { 'Authorization': `Bearer ${apiKey}` } },
8790
cacheKey: 'openai',
8891
};

containers/api-proxy/proxy-utils.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,17 @@ function normalizeApiTarget(value) {
3131
const parsed = new URL(candidate);
3232

3333
if (parsed.pathname !== '/' || parsed.search || parsed.hash || parsed.username || parsed.password || parsed.port) {
34+
const safe = trimmed.replace(/[\x00-\x1f\x7f]/g, '?');
3435
console.warn(
35-
`Ignoring unsupported API target URL components in ${trimmed}; ` +
36+
`Ignoring unsupported API target URL components in ${safe}; ` +
3637
'configure path prefixes via the corresponding *_API_BASE_PATH environment variable.'
3738
);
3839
}
3940

4041
return parsed.hostname || undefined;
4142
} catch (err) {
42-
console.warn(`Invalid API target ${trimmed}; expected a hostname (e.g. 'api.example.com') or URL`);
43+
const safe = trimmed.replace(/[\x00-\x1f\x7f]/g, '?');
44+
console.warn(`Invalid API target ${safe}; expected a hostname (e.g. 'api.example.com') or URL`);
4345
return undefined;
4446
}
4547
}

containers/api-proxy/server.test.js

Lines changed: 239 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ const { deriveCopilotApiTarget, deriveGitHubApiTarget, deriveGitHubApiBasePath,
1515
const { resolveOpenCodeRoute } = require('./providers/opencode');
1616

1717
// Core proxy functions that remain in server.js
18-
const { proxyWebSocket, httpProbe, validateApiKeys, keyValidationResults, resetKeyValidationState, fetchJson, extractModelIds, fetchStartupModels, reflectEndpoints, healthResponse, cachedModels, resetModelCacheState, makeModelBodyTransform, MODEL_ALIASES, buildModelsJson, writeModelsJson } = require('./server');
18+
const { proxyWebSocket, httpProbe, validateApiKeys, keyValidationResults, resetKeyValidationState, fetchJson, extractModelIds, fetchStartupModels, reflectEndpoints, healthResponse, cachedModels, resetModelCacheState, makeModelBodyTransform, MODEL_ALIASES, buildModelsJson, writeModelsJson, createProviderServer } = require('./server');
1919

2020
describe('normalizeApiTarget', () => {
2121
it('should strip https:// prefix', () => {
@@ -2125,3 +2125,241 @@ describe('composeBodyTransforms', () => {
21252125
expect(composed(Buffer.from('hello'))).toBeNull();
21262126
});
21272127
});
2128+
2129+
// ── createProviderServer tests ────────────────────────────────────────────────
2130+
//
2131+
// Tests that verify the generic proxy server factory honours the ProviderAdapter
2132+
// interface: health routing, unconfigured-stub responses, URL transforms, and
2133+
// adapter-specific auth selection.
2134+
//
2135+
describe('createProviderServer', () => {
2136+
const servers = [];
2137+
2138+
/** Small helper: start a createProviderServer instance and return its port. */
2139+
function startAdapter(adapter) {
2140+
return new Promise((resolve) => {
2141+
const srv = createProviderServer(adapter);
2142+
srv.listen(0, '127.0.0.1', () => {
2143+
servers.push(srv);
2144+
resolve(srv.address().port);
2145+
});
2146+
});
2147+
}
2148+
2149+
/** Fetch a path from a server running on localhost and return { status, body }. */
2150+
function fetch(port, path, opts = {}) {
2151+
return new Promise((resolve, reject) => {
2152+
const req = http.request(
2153+
{ hostname: '127.0.0.1', port, path, method: opts.method || 'GET', headers: opts.headers || {} },
2154+
(res) => {
2155+
let data = '';
2156+
res.on('data', (c) => { data += c; });
2157+
res.on('end', () => {
2158+
let parsed;
2159+
try { parsed = JSON.parse(data); } catch { parsed = data; }
2160+
resolve({ status: res.statusCode, body: parsed, headers: res.headers });
2161+
});
2162+
}
2163+
);
2164+
req.on('error', reject);
2165+
if (opts.body) req.write(opts.body);
2166+
req.end();
2167+
});
2168+
}
2169+
2170+
afterEach((done) => {
2171+
let remaining = servers.length;
2172+
if (!remaining) { done(); return; }
2173+
servers.splice(0).forEach((s) => s.close(() => { if (!--remaining) done(); }));
2174+
});
2175+
2176+
// ── /health endpoint — enabled adapter ──────────────────────────────────────
2177+
2178+
it('returns 200 /health when adapter is enabled', async () => {
2179+
const adapter = {
2180+
name: 'test-enabled', port: 0, isManagementPort: false, alwaysBind: false,
2181+
participatesInValidation: false,
2182+
isEnabled: () => true,
2183+
getTargetHost: () => 'api.example.com',
2184+
getBasePath: () => '',
2185+
getAuthHeaders: () => ({}),
2186+
getBodyTransform: () => null,
2187+
};
2188+
const port = await startAdapter(adapter);
2189+
const { status, body } = await fetch(port, '/health');
2190+
expect(status).toBe(200);
2191+
expect(body.status).toBe('healthy');
2192+
expect(body.service).toBe('awf-api-proxy-test-enabled');
2193+
});
2194+
2195+
// ── /health endpoint — disabled adapter (default 503) ───────────────────────
2196+
2197+
it('returns default 503 /health when adapter is disabled and has no getUnconfiguredHealthResponse', async () => {
2198+
const adapter = {
2199+
name: 'test-disabled', port: 0, isManagementPort: false, alwaysBind: true,
2200+
participatesInValidation: false,
2201+
isEnabled: () => false,
2202+
getTargetHost: () => '',
2203+
getBasePath: () => '',
2204+
getAuthHeaders: () => ({}),
2205+
getBodyTransform: () => null,
2206+
getUnconfiguredResponse: () => ({ statusCode: 503, body: { error: 'not configured' } }),
2207+
};
2208+
const port = await startAdapter(adapter);
2209+
const { status, body } = await fetch(port, '/health');
2210+
expect(status).toBe(503);
2211+
expect(body.status).toBe('not_configured');
2212+
expect(body.service).toBe('awf-api-proxy-test-disabled');
2213+
});
2214+
2215+
// ── /health endpoint — custom unconfigured health response ──────────────────
2216+
2217+
it('returns custom getUnconfiguredHealthResponse when adapter is disabled', async () => {
2218+
const adapter = {
2219+
name: 'test-custom-health', port: 0, isManagementPort: false, alwaysBind: true,
2220+
participatesInValidation: false,
2221+
isEnabled: () => false,
2222+
getTargetHost: () => '',
2223+
getBasePath: () => '',
2224+
getAuthHeaders: () => ({}),
2225+
getBodyTransform: () => null,
2226+
getUnconfiguredResponse: () => ({ statusCode: 503, body: { error: 'not configured' } }),
2227+
getUnconfiguredHealthResponse: () => ({
2228+
statusCode: 503,
2229+
body: { status: 'not_configured', service: 'awf-api-proxy-gemini', error: 'GEMINI_API_KEY not configured' },
2230+
}),
2231+
};
2232+
const port = await startAdapter(adapter);
2233+
const { status, body } = await fetch(port, '/health');
2234+
expect(status).toBe(503);
2235+
expect(body.service).toBe('awf-api-proxy-gemini');
2236+
expect(body.error).toMatch(/GEMINI_API_KEY/);
2237+
});
2238+
2239+
// ── Unconfigured stub — non-health request ────────────────────────────────
2240+
2241+
it('returns getUnconfiguredResponse body for proxy requests when disabled', async () => {
2242+
const adapter = {
2243+
name: 'test-unconfigured', port: 0, isManagementPort: false, alwaysBind: true,
2244+
participatesInValidation: false,
2245+
isEnabled: () => false,
2246+
getTargetHost: () => '',
2247+
getBasePath: () => '',
2248+
getAuthHeaders: () => ({}),
2249+
getBodyTransform: () => null,
2250+
getUnconfiguredResponse: () => ({
2251+
statusCode: 503,
2252+
body: { error: 'proxy not configured (no API key)' },
2253+
}),
2254+
};
2255+
const port = await startAdapter(adapter);
2256+
const { status, body } = await fetch(port, '/v1/chat/completions', { method: 'POST', body: '{}' });
2257+
expect(status).toBe(503);
2258+
expect(body.error).toMatch(/proxy not configured/);
2259+
});
2260+
2261+
it('returns default 503 for proxy requests when disabled and no getUnconfiguredResponse', async () => {
2262+
const adapter = {
2263+
name: 'test-no-stub', port: 0, isManagementPort: false, alwaysBind: false,
2264+
participatesInValidation: false,
2265+
isEnabled: () => false,
2266+
getTargetHost: () => '',
2267+
getBasePath: () => '',
2268+
getAuthHeaders: () => ({}),
2269+
getBodyTransform: () => null,
2270+
};
2271+
const port = await startAdapter(adapter);
2272+
const { status, body } = await fetch(port, '/v1/models', { method: 'GET' });
2273+
expect(status).toBe(503);
2274+
expect(body.error).toMatch(/test-no-stub.*not configured/);
2275+
});
2276+
2277+
// ── URL transform ─────────────────────────────────────────────────────────
2278+
2279+
it('applies transformRequestUrl before proxying', async () => {
2280+
// Record what the transform was called with; upstream will fail (no real host)
2281+
// but the transform runs synchronously in the request handler before proxying starts.
2282+
const calls = [];
2283+
const adapter = {
2284+
name: 'test-url-transform', port: 0, isManagementPort: false, alwaysBind: false,
2285+
participatesInValidation: false,
2286+
isEnabled: () => true,
2287+
getTargetHost: () => 'api.example.com',
2288+
getBasePath: () => '',
2289+
getAuthHeaders: () => ({}),
2290+
getBodyTransform: () => null,
2291+
transformRequestUrl: (url) => {
2292+
const result = url.replace('?key=placeholder', '');
2293+
calls.push({ input: url, output: result });
2294+
return result;
2295+
},
2296+
};
2297+
const port = await startAdapter(adapter);
2298+
// fetch will return a non-2xx (proxy can't reach api.example.com in test), that's fine.
2299+
await fetch(port, '/v1/models?key=placeholder').catch(() => {});
2300+
expect(calls).toHaveLength(1);
2301+
expect(calls[0].input).toBe('/v1/models?key=placeholder');
2302+
expect(calls[0].output).toBe('/v1/models');
2303+
});
2304+
2305+
// ── Auth headers ──────────────────────────────────────────────────────────
2306+
2307+
it('calls getAuthHeaders() for each proxied request', async () => {
2308+
// Record the headers returned by getAuthHeaders; upstream will fail (no real host)
2309+
// but getAuthHeaders is called synchronously in the request handler.
2310+
const headerCalls = [];
2311+
const adapter = {
2312+
name: 'test-auth', port: 0, isManagementPort: false, alwaysBind: false,
2313+
participatesInValidation: false,
2314+
isEnabled: () => true,
2315+
getTargetHost: () => 'api.example.com',
2316+
getBasePath: () => '',
2317+
getAuthHeaders: (req) => {
2318+
const h = { 'Authorization': 'Bearer injected-token' };
2319+
headerCalls.push(h);
2320+
return h;
2321+
},
2322+
getBodyTransform: () => null,
2323+
};
2324+
const port = await startAdapter(adapter);
2325+
await fetch(port, '/v1/models').catch(() => {});
2326+
expect(headerCalls).toHaveLength(1);
2327+
expect(headerCalls[0].Authorization).toBe('Bearer injected-token');
2328+
});
2329+
2330+
// ── getBodyTransform called once per request (not per-call) ──────────────
2331+
2332+
it('calls getBodyTransform() once per request', async () => {
2333+
let callCount = 0;
2334+
const upstream = http.createServer((req, res) => {
2335+
req.resume();
2336+
res.writeHead(200, { 'Content-Type': 'application/json' });
2337+
res.end('{}');
2338+
});
2339+
const upstreamPort = await new Promise((resolve) => {
2340+
upstream.listen(0, '127.0.0.1', () => resolve(upstream.address().port));
2341+
});
2342+
servers.push(upstream);
2343+
2344+
const adapter = {
2345+
name: 'test-transform-count', port: 0, isManagementPort: false, alwaysBind: false,
2346+
participatesInValidation: false,
2347+
isEnabled: () => true,
2348+
getTargetHost: () => `127.0.0.1:${upstreamPort}`,
2349+
getBasePath: () => '',
2350+
getAuthHeaders: () => ({}),
2351+
getBodyTransform: () => { callCount++; return null; },
2352+
};
2353+
const port = await startAdapter(adapter);
2354+
2355+
await new Promise((resolve, reject) => {
2356+
const req = http.request({ hostname: '127.0.0.1', port, path: '/v1/chat/completions', method: 'POST' }, resolve);
2357+
req.on('error', reject);
2358+
req.write('{}');
2359+
req.end();
2360+
});
2361+
2362+
await new Promise((r) => setTimeout(r, 100));
2363+
expect(callCount).toBe(1);
2364+
});
2365+
});

0 commit comments

Comments
 (0)