Skip to content

Commit 827592b

Browse files
CopilotlpcoxCopilotCopilot
authored
fix(api-proxy): fix Gemini API_KEY_INVALID with credential isolation (#1995)
* Initial plan * fix(api-proxy): fix Gemini API_KEY_INVALID errors - Add x-goog-api-key to STRIPPED_HEADERS to ensure placeholder is always stripped before the real key is injected - Add stripGeminiKeyParam() to remove ?key= query params from URLs (the @google/genai SDK may append key= in addition to the header) - Apply stripGeminiKeyParam() in both HTTP and WebSocket Gemini handlers - Extend auth_inject debug logging to cover x-goog-api-key - Export shouldStripHeader and stripGeminiKeyParam for unit testing - Add tests for shouldStripHeader and stripGeminiKeyParam Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/e48bf273-4302-49fe-acde-42cbd46c679c * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix: case-insensitive auth header lookup for debug logging Address PR review feedback: - Make injected-key detection case-insensitive so auth_inject debug logs fire for OpenAI/Copilot (which use capital-A 'Authorization') in addition to Anthropic and Gemini. - Clarify stripGeminiKeyParam guard comments explaining why absolute/protocol-relative URLs are rejected before parsing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Landon Cox <landon.cox@microsoft.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 16c20a7 commit 827592b

2 files changed

Lines changed: 103 additions & 3 deletions

File tree

containers/api-proxy/server.js

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ const STRIPPED_HEADERS = new Set([
4646
'authorization',
4747
'proxy-authorization',
4848
'x-api-key',
49+
'x-goog-api-key',
4950
'forwarded',
5051
'via',
5152
]);
@@ -167,6 +168,32 @@ function buildUpstreamPath(reqUrl, targetHost, basePath) {
167168
return prefix + targetUrl.pathname + targetUrl.search;
168169
}
169170

171+
/**
172+
* Strip the `key` query parameter from a Gemini request URL.
173+
*
174+
* The @google/genai SDK (and older Gemini SDK versions) may append `?key=<value>`
175+
* to every request URL in addition to setting the `x-goog-api-key` header.
176+
* The proxy injects the real key via the header, so the placeholder `key=`
177+
* value must be removed before forwarding to Google to prevent
178+
* API_KEY_INVALID errors.
179+
*
180+
* @param {string} reqUrl - The incoming request URL (must start with exactly one '/')
181+
* @returns {string} URL with the `key` query parameter removed
182+
*/
183+
function stripGeminiKeyParam(reqUrl) {
184+
// Only operate on relative request paths that begin with exactly one slash.
185+
// Returning other inputs unchanged lets proxyRequest's relative-URL check reject them.
186+
// The guard prevents absolute URLs (e.g. 'http://evil.com/path?key=…') and
187+
// protocol-relative URLs ('//host/path') from being normalized into a relative path.
188+
if (typeof reqUrl !== 'string' || !reqUrl.startsWith('/') || reqUrl.startsWith('//')) {
189+
return reqUrl;
190+
}
191+
const parsed = new URL(reqUrl, 'http://localhost');
192+
parsed.searchParams.delete('key');
193+
// Reconstruct relative path only — never emit the scheme/host from the dummy base.
194+
return parsed.pathname + parsed.search;
195+
}
196+
170197
// Optional base path prefixes for API targets (e.g. /serving-endpoints for Databricks)
171198
const OPENAI_API_BASE_PATH = normalizeBasePath(process.env.OPENAI_API_BASE_PATH);
172199
const ANTHROPIC_API_BASE_PATH = normalizeBasePath(process.env.ANTHROPIC_API_BASE_PATH);
@@ -485,7 +512,8 @@ function proxyRequest(req, res, targetHost, injectHeaders, provider, basePath =
485512
Object.assign(headers, injectHeaders);
486513

487514
// Log auth header injection for debugging credential-isolation issues
488-
const injectedKey = injectHeaders['x-api-key'] || injectHeaders['authorization'];
515+
// Use case-insensitive lookup since providers use mixed casing (e.g. 'Authorization' vs 'authorization')
516+
const injectedKey = Object.entries(injectHeaders).find(([k]) => ['x-api-key', 'authorization', 'x-goog-api-key'].includes(k.toLowerCase()))?.[1];
489517
if (injectedKey) {
490518
const keyPreview = injectedKey.length > 8
491519
? `${injectedKey.substring(0, 8)}...${injectedKey.substring(injectedKey.length - 4)}`
@@ -1016,12 +1044,18 @@ if (require.main === module) {
10161044
const contentLength = parseInt(req.headers['content-length'], 10) || 0;
10171045
if (checkRateLimit(req, res, 'gemini', contentLength)) return;
10181046

1047+
// Strip any ?key= query parameter — the @google/genai SDK may append it to the URL.
1048+
// The proxy injects the real key via x-goog-api-key header instead.
1049+
req.url = stripGeminiKeyParam(req.url);
1050+
10191051
proxyRequest(req, res, GEMINI_API_TARGET, {
10201052
'x-goog-api-key': GEMINI_API_KEY,
10211053
}, 'gemini', GEMINI_API_BASE_PATH);
10221054
});
10231055

10241056
geminiServer.on('upgrade', (req, socket, head) => {
1057+
// Strip any ?key= query parameter — the @google/genai SDK may append it to the URL.
1058+
req.url = stripGeminiKeyParam(req.url);
10251059
proxyWebSocket(req, socket, head, GEMINI_API_TARGET, {
10261060
'x-goog-api-key': GEMINI_API_KEY,
10271061
}, 'gemini', GEMINI_API_BASE_PATH);
@@ -1155,4 +1189,4 @@ if (require.main === module) {
11551189
}
11561190

11571191
// Export for testing
1158-
module.exports = { normalizeApiTarget, deriveCopilotApiTarget, deriveGitHubApiTarget, deriveGitHubApiBasePath, normalizeBasePath, buildUpstreamPath, proxyWebSocket, resolveCopilotAuthToken, resolveOpenCodeRoute };
1192+
module.exports = { normalizeApiTarget, deriveCopilotApiTarget, deriveGitHubApiTarget, deriveGitHubApiBasePath, normalizeBasePath, buildUpstreamPath, proxyWebSocket, resolveCopilotAuthToken, resolveOpenCodeRoute, shouldStripHeader, stripGeminiKeyParam };

containers/api-proxy/server.test.js

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
const http = require('http');
66
const tls = require('tls');
77
const { EventEmitter } = require('events');
8-
const { normalizeApiTarget, deriveCopilotApiTarget, deriveGitHubApiTarget, deriveGitHubApiBasePath, normalizeBasePath, buildUpstreamPath, proxyWebSocket, resolveCopilotAuthToken, resolveOpenCodeRoute } = require('./server');
8+
const { normalizeApiTarget, deriveCopilotApiTarget, deriveGitHubApiTarget, deriveGitHubApiBasePath, normalizeBasePath, buildUpstreamPath, proxyWebSocket, resolveCopilotAuthToken, resolveOpenCodeRoute, shouldStripHeader, stripGeminiKeyParam } = require('./server');
99

1010
describe('normalizeApiTarget', () => {
1111
it('should strip https:// prefix', () => {
@@ -449,6 +449,72 @@ describe('buildUpstreamPath', () => {
449449
});
450450
});
451451

452+
describe('shouldStripHeader', () => {
453+
it('should strip authorization header', () => {
454+
expect(shouldStripHeader('authorization')).toBe(true);
455+
expect(shouldStripHeader('Authorization')).toBe(true);
456+
});
457+
458+
it('should strip x-api-key header', () => {
459+
expect(shouldStripHeader('x-api-key')).toBe(true);
460+
expect(shouldStripHeader('X-Api-Key')).toBe(true);
461+
});
462+
463+
it('should strip x-goog-api-key header (Gemini placeholder must be stripped)', () => {
464+
expect(shouldStripHeader('x-goog-api-key')).toBe(true);
465+
expect(shouldStripHeader('X-Goog-Api-Key')).toBe(true);
466+
});
467+
468+
it('should strip proxy-authorization header', () => {
469+
expect(shouldStripHeader('proxy-authorization')).toBe(true);
470+
});
471+
472+
it('should strip x-forwarded-* headers', () => {
473+
expect(shouldStripHeader('x-forwarded-for')).toBe(true);
474+
expect(shouldStripHeader('x-forwarded-host')).toBe(true);
475+
});
476+
477+
it('should not strip content-type header', () => {
478+
expect(shouldStripHeader('content-type')).toBe(false);
479+
});
480+
481+
it('should not strip anthropic-version header', () => {
482+
expect(shouldStripHeader('anthropic-version')).toBe(false);
483+
});
484+
});
485+
486+
describe('stripGeminiKeyParam', () => {
487+
it('should remove the key= query parameter', () => {
488+
expect(stripGeminiKeyParam('/v1/models/gemini-pro:generateContent?key=placeholder'))
489+
.toBe('/v1/models/gemini-pro:generateContent');
490+
});
491+
492+
it('should remove key= while preserving other query parameters', () => {
493+
expect(stripGeminiKeyParam('/v1/models/gemini-pro:generateContent?key=placeholder&alt=json'))
494+
.toBe('/v1/models/gemini-pro:generateContent?alt=json');
495+
});
496+
497+
it('should return path unchanged when no key= parameter is present', () => {
498+
expect(stripGeminiKeyParam('/v1/models/gemini-pro:generateContent'))
499+
.toBe('/v1/models/gemini-pro:generateContent');
500+
});
501+
502+
it('should return path unchanged when only unrelated query parameters exist', () => {
503+
expect(stripGeminiKeyParam('/v1/models/gemini-pro:generateContent?alt=json&stream=true'))
504+
.toBe('/v1/models/gemini-pro:generateContent?alt=json&stream=true');
505+
});
506+
507+
it('should handle root path without key param', () => {
508+
expect(stripGeminiKeyParam('/')).toBe('/');
509+
});
510+
511+
it('should handle path with only key= param, leaving no trailing ?', () => {
512+
// URL.search returns '' when no params remain after deletion
513+
const result = stripGeminiKeyParam('/v1/generateContent?key=abc');
514+
expect(result).toBe('/v1/generateContent');
515+
});
516+
});
517+
452518
// ── Helpers for proxyWebSocket tests ──────────────────────────────────────────
453519

454520
/** Create a minimal mock socket with write/destroy spies. */

0 commit comments

Comments
 (0)