Skip to content

Commit 899ee4b

Browse files
Copilotlpcox
andauthored
fix(api-proxy): strip accidental "Bearer " prefix in BYOK Copilot auth token (#2521)
* Initial plan * fix: prevent double Bearer prefix in BYOK copilot auth header - Strip accidental "Bearer " prefix from COPILOT_API_KEY/COPILOT_GITHUB_TOKEN in resolveCopilotAuthToken() and createCopilotAdapter() to prevent double-prefixed "Authorization: Bearer Bearer <key>" rejected by external providers (e.g. OpenRouter) with 400 "badly formatted" error - Extend upstream_auth_error log to also fire on HTTP 400 with BYOK hint - Add unit tests: Bearer-prefix stripping, BYOK getAuthHeaders format, 400/401 upstream_auth_error log events Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/f4fac661-5248-46ac-919a-4f77d41f89a0 * refactor: address review feedback on BYOK auth header fix - Extract stripBearerPrefix() as a named module-level helper in copilot.js, reused by both resolveCopilotAuthToken() and createCopilotAdapter() — eliminates the duplicated inline `strip` function; export for direct unit testing - Replace Copilot/BYOK-specific HTTP 400 message in proxyRequest with a generic "check that the API key is valid and correctly formatted" message that is accurate for all providers - Fix server.test.js BYOK mode comment to reference COPILOT_API_KEY (the sidecar's actual input) rather than COPILOT_PROVIDER_API_KEY (the agent-side placeholder) - Add dedicated describe('stripBearerPrefix') unit tests for the new helper; simplify resolveCopilotAuthToken integration tests Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/fcc4060e-94ff-474f-8254-d8b81acd6204 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
1 parent f03c273 commit 899ee4b

3 files changed

Lines changed: 261 additions & 8 deletions

File tree

containers/api-proxy/providers/copilot.js

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,35 @@
1717
const { normalizeApiTarget } = require('../proxy-utils');
1818
const { URL } = require('url');
1919

20+
/**
21+
* Strip any accidental "Bearer " prefix from a raw credential value and trim
22+
* surrounding whitespace. Returns undefined when the result is empty so that
23+
* callers can use `|| undefined` fall-through cleanly.
24+
*
25+
* A value like "Bearer " (prefix with nothing after it) reduces to undefined
26+
* rather than "Bearer", which is why the prefix is removed before trimming.
27+
*
28+
* @param {string|undefined} value - Raw credential string
29+
* @returns {string|undefined}
30+
*/
31+
function stripBearerPrefix(value) {
32+
return ((value || '').replace(/^\s*Bearer\s+/i, '').trim()) || undefined;
33+
}
34+
2035
/**
2136
* Resolves the Copilot auth token from environment variables.
2237
* COPILOT_GITHUB_TOKEN (GitHub OAuth) takes precedence over COPILOT_API_KEY (direct key).
2338
*
39+
* Any accidental "Bearer " prefix is stripped via stripBearerPrefix so that
40+
* the injected Authorization header is exactly "Bearer <token>" rather than
41+
* the double-prefixed "Bearer Bearer <token>" that would be rejected by
42+
* external providers in BYOK mode.
43+
*
2444
* @param {Record<string, string|undefined>} env - Environment variables to inspect
2545
* @returns {string|undefined} The resolved auth token, or undefined if neither is set
2646
*/
2747
function resolveCopilotAuthToken(env = process.env) {
28-
const githubToken = (env.COPILOT_GITHUB_TOKEN || '').trim() || undefined;
29-
const apiKey = (env.COPILOT_API_KEY || '').trim() || undefined;
30-
return githubToken || apiKey;
48+
return stripBearerPrefix(env.COPILOT_GITHUB_TOKEN) || stripBearerPrefix(env.COPILOT_API_KEY);
3149
}
3250

3351
/**
@@ -127,8 +145,8 @@ function deriveGitHubApiBasePath(env = process.env) {
127145
* @returns {import('./index').ProviderAdapter}
128146
*/
129147
function createCopilotAdapter(env, deps = {}) {
130-
const githubToken = (env.COPILOT_GITHUB_TOKEN || '').trim() || undefined;
131-
const apiKey = (env.COPILOT_API_KEY || '').trim() || undefined;
148+
const githubToken = stripBearerPrefix(env.COPILOT_GITHUB_TOKEN);
149+
const apiKey = stripBearerPrefix(env.COPILOT_API_KEY);
132150
const authToken = resolveCopilotAuthToken(env);
133151
const integrationId = env.COPILOT_INTEGRATION_ID || 'copilot-developer-cli';
134152
const rawTarget = deriveCopilotApiTarget(env);
@@ -245,6 +263,7 @@ function createCopilotAdapter(env, deps = {}) {
245263
module.exports = {
246264
createCopilotAdapter,
247265
resolveCopilotAuthToken,
266+
stripBearerPrefix,
248267
deriveCopilotApiTarget,
249268
deriveGitHubApiTarget,
250269
deriveGitHubApiBasePath,

containers/api-proxy/server.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -379,11 +379,11 @@ function proxyRequest(req, res, targetHost, injectHeaders, provider, basePath =
379379

380380
const resHeaders = { ...proxyRes.headers, 'x-request-id': requestId };
381381

382-
if (proxyRes.statusCode === 401 || proxyRes.statusCode === 403) {
382+
if (proxyRes.statusCode === 400 || proxyRes.statusCode === 401 || proxyRes.statusCode === 403) {
383383
logRequest('warn', 'upstream_auth_error', {
384384
request_id: requestId, provider, status: proxyRes.statusCode,
385385
upstream_host: targetHost, path: sanitizeForLog(req.url),
386-
message: `Upstream returned ${proxyRes.statusCode} — check that the API key is valid and has not expired`,
386+
message: `Upstream returned ${proxyRes.statusCode} — check that the API key is valid and correctly formatted`,
387387
});
388388
}
389389

containers/api-proxy/server.test.js

Lines changed: 235 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ const { EventEmitter } = require('events');
1111
const { normalizeApiTarget, normalizeBasePath, buildUpstreamPath, shouldStripHeader, stripGeminiKeyParam, composeBodyTransforms } = require('./proxy-utils');
1212

1313
// Provider-specific functions that live in their respective adapter modules
14-
const { deriveCopilotApiTarget, deriveGitHubApiTarget, deriveGitHubApiBasePath, resolveCopilotAuthToken } = require('./providers/copilot');
14+
const { deriveCopilotApiTarget, deriveGitHubApiTarget, deriveGitHubApiBasePath, resolveCopilotAuthToken, stripBearerPrefix, createCopilotAdapter } = require('./providers/copilot');
1515
const { resolveOpenCodeRoute } = require('./providers/opencode');
1616

1717
// Core proxy functions that remain in server.js
@@ -883,6 +883,45 @@ describe('proxyWebSocket', () => {
883883
});
884884
});
885885

886+
describe('stripBearerPrefix', () => {
887+
it('strips "Bearer " prefix from a token value', () => {
888+
expect(stripBearerPrefix('Bearer sk-or-v1-abc')).toBe('sk-or-v1-abc');
889+
});
890+
891+
it('strips "Bearer " prefix case-insensitively', () => {
892+
expect(stripBearerPrefix('bearer sk-or-v1-abc')).toBe('sk-or-v1-abc');
893+
expect(stripBearerPrefix('BEARER sk-or-v1-abc')).toBe('sk-or-v1-abc');
894+
});
895+
896+
it('strips leading whitespace before "Bearer "', () => {
897+
expect(stripBearerPrefix(' Bearer sk-or-v1-abc')).toBe('sk-or-v1-abc');
898+
});
899+
900+
it('returns value unchanged when no "Bearer " prefix is present', () => {
901+
expect(stripBearerPrefix('sk-or-v1-abc')).toBe('sk-or-v1-abc');
902+
expect(stripBearerPrefix('gho_abc123')).toBe('gho_abc123');
903+
});
904+
905+
it('does not strip "Bearer" without a following space', () => {
906+
expect(stripBearerPrefix('BearerToken123')).toBe('BearerToken123');
907+
});
908+
909+
it('returns undefined when value is only "Bearer " (nothing after prefix)', () => {
910+
expect(stripBearerPrefix('Bearer ')).toBeUndefined();
911+
expect(stripBearerPrefix('Bearer ')).toBeUndefined();
912+
});
913+
914+
it('returns undefined for empty or whitespace-only input', () => {
915+
expect(stripBearerPrefix('')).toBeUndefined();
916+
expect(stripBearerPrefix(' ')).toBeUndefined();
917+
expect(stripBearerPrefix(undefined)).toBeUndefined();
918+
});
919+
920+
it('trims surrounding whitespace from the token', () => {
921+
expect(stripBearerPrefix(' sk-or-v1-abc ')).toBe('sk-or-v1-abc');
922+
});
923+
});
924+
886925
describe('resolveCopilotAuthToken', () => {
887926
it('should return COPILOT_GITHUB_TOKEN when only it is set', () => {
888927
expect(resolveCopilotAuthToken({ COPILOT_GITHUB_TOKEN: 'gho_abc123' })).toBe('gho_abc123');
@@ -921,6 +960,94 @@ describe('resolveCopilotAuthToken', () => {
921960
COPILOT_API_KEY: 'sk-byok-key',
922961
})).toBe('sk-byok-key');
923962
});
963+
964+
// Integration: verify that Bearer-prefix stripping (via stripBearerPrefix) is
965+
// applied to both token sources when resolving.
966+
967+
it('strips "Bearer " prefix from COPILOT_API_KEY when resolving', () => {
968+
expect(resolveCopilotAuthToken({ COPILOT_API_KEY: 'Bearer sk-or-v1-abc' })).toBe('sk-or-v1-abc');
969+
});
970+
971+
it('strips "Bearer " prefix from COPILOT_GITHUB_TOKEN when resolving', () => {
972+
expect(resolveCopilotAuthToken({ COPILOT_GITHUB_TOKEN: 'Bearer gho_abc123' })).toBe('gho_abc123');
973+
});
974+
975+
it('prefers stripped COPILOT_GITHUB_TOKEN over stripped COPILOT_API_KEY', () => {
976+
expect(resolveCopilotAuthToken({
977+
COPILOT_GITHUB_TOKEN: 'Bearer gho_abc123',
978+
COPILOT_API_KEY: 'Bearer sk-byok-key',
979+
})).toBe('gho_abc123');
980+
});
981+
});
982+
983+
// ── createCopilotAdapter — BYOK auth header format ───────────────────────────
984+
//
985+
// These tests guard against the "badly formatted Authorization header" bug in
986+
// BYOK mode where the sidecar is configured with COPILOT_API_KEY (the real key
987+
// held by the sidecar) and could produce "Authorization: Bearer Bearer <key>"
988+
// if the COPILOT_API_KEY value already contained the "Bearer " prefix.
989+
// They also verify that the header injected for inference requests is exactly
990+
// "Bearer <key>" and that the Copilot-Integration-Id header is present.
991+
992+
describe('createCopilotAdapter — BYOK getAuthHeaders', () => {
993+
const fakeReq = { url: '/v1/chat/completions', method: 'POST', headers: {} };
994+
const fakeModelsReq = { url: '/models', method: 'GET', headers: {} };
995+
996+
it('injects Authorization: Bearer <key> for BYOK inference request', () => {
997+
const adapter = createCopilotAdapter({ COPILOT_API_KEY: 'sk-or-v1-abc123' });
998+
const headers = adapter.getAuthHeaders(fakeReq);
999+
expect(headers['Authorization']).toBe('Bearer sk-or-v1-abc123');
1000+
});
1001+
1002+
it('injects Copilot-Integration-Id header for BYOK inference request', () => {
1003+
const adapter = createCopilotAdapter({ COPILOT_API_KEY: 'sk-or-v1-abc123' });
1004+
const headers = adapter.getAuthHeaders(fakeReq);
1005+
expect(headers['Copilot-Integration-Id']).toBe('copilot-developer-cli');
1006+
});
1007+
1008+
it('prevents double "Bearer " prefix when API key already contains "Bearer " prefix (BYOK bug fix)', () => {
1009+
const adapter = createCopilotAdapter({ COPILOT_API_KEY: 'Bearer sk-or-v1-abc123' });
1010+
const headers = adapter.getAuthHeaders(fakeReq);
1011+
// Must NOT be "Bearer Bearer sk-or-v1-abc123"
1012+
expect(headers['Authorization']).toBe('Bearer sk-or-v1-abc123');
1013+
expect(headers['Authorization']).not.toContain('Bearer Bearer');
1014+
});
1015+
1016+
it('strips "Bearer " prefix case-insensitively from API key', () => {
1017+
const adapter = createCopilotAdapter({ COPILOT_API_KEY: 'BEARER sk-or-v1-abc123' });
1018+
const headers = adapter.getAuthHeaders(fakeReq);
1019+
expect(headers['Authorization']).toBe('Bearer sk-or-v1-abc123');
1020+
});
1021+
1022+
it('uses COPILOT_GITHUB_TOKEN (not COPILOT_API_KEY) for /models GET in BYOK+token mode', () => {
1023+
const adapter = createCopilotAdapter({
1024+
COPILOT_GITHUB_TOKEN: 'gho_oauth_token',
1025+
COPILOT_API_KEY: 'sk-or-v1-abc123',
1026+
});
1027+
const headers = adapter.getAuthHeaders(fakeModelsReq);
1028+
expect(headers['Authorization']).toBe('Bearer gho_oauth_token');
1029+
});
1030+
1031+
it('uses API key for /models GET when no GITHUB_TOKEN is set (BYOK-only mode)', () => {
1032+
const adapter = createCopilotAdapter({ COPILOT_API_KEY: 'sk-or-v1-abc123' });
1033+
const headers = adapter.getAuthHeaders(fakeModelsReq);
1034+
// In BYOK-only mode, githubToken is undefined so falls through to authToken (apiKey)
1035+
expect(headers['Authorization']).toBe('Bearer sk-or-v1-abc123');
1036+
});
1037+
1038+
it('is enabled when only COPILOT_API_KEY is set', () => {
1039+
const adapter = createCopilotAdapter({ COPILOT_API_KEY: 'sk-or-v1-abc123' });
1040+
expect(adapter.isEnabled()).toBe(true);
1041+
});
1042+
1043+
it('uses custom COPILOT_INTEGRATION_ID when set', () => {
1044+
const adapter = createCopilotAdapter({
1045+
COPILOT_API_KEY: 'sk-or-v1-abc123',
1046+
COPILOT_INTEGRATION_ID: 'my-custom-integration',
1047+
});
1048+
const headers = adapter.getAuthHeaders(fakeReq);
1049+
expect(headers['Copilot-Integration-Id']).toBe('my-custom-integration');
1050+
});
9241051
});
9251052

9261053
describe('resolveOpenCodeRoute', () => {
@@ -2362,4 +2489,111 @@ describe('createProviderServer', () => {
23622489
await new Promise((r) => setTimeout(r, 100));
23632490
expect(callCount).toBe(1);
23642491
});
2492+
2493+
// ── 400/401/403 upstream response → upstream_auth_error log ──────────────
2494+
//
2495+
// When the upstream provider returns an auth-related error status, the proxy
2496+
// must emit an 'upstream_auth_error' log event so operators can diagnose
2497+
// credential problems quickly. A 400 specifically indicates a possible
2498+
// malformed Authorization header (e.g. double "Bearer " prefix in BYOK mode).
2499+
2500+
/**
2501+
* Build a minimal mock for https.request that immediately calls back with a
2502+
* response of the given status code. The mock proxyRes emits 'end' after
2503+
* the callback so request_complete is also logged.
2504+
*/
2505+
function mockHttpsWithStatus(statusCode) {
2506+
return jest.spyOn(https, 'request').mockImplementation((options, callback) => {
2507+
const proxyReq = new EventEmitter();
2508+
proxyReq.write = jest.fn();
2509+
proxyReq.end = jest.fn(() => {
2510+
setImmediate(() => {
2511+
const proxyRes = new EventEmitter();
2512+
proxyRes.statusCode = statusCode;
2513+
proxyRes.headers = { 'content-type': 'application/json' };
2514+
proxyRes.pipe = jest.fn((destRes) => { destRes.end('{}'); });
2515+
callback(proxyRes);
2516+
setImmediate(() => proxyRes.emit('end'));
2517+
});
2518+
});
2519+
proxyReq.destroy = jest.fn();
2520+
return proxyReq;
2521+
});
2522+
}
2523+
2524+
it('emits upstream_auth_error when upstream returns 400', async () => {
2525+
const { lines, spy } = collectLogOutput();
2526+
mockHttpsWithStatus(400);
2527+
2528+
const adapter = {
2529+
name: 'copilot', port: 0, isManagementPort: false, alwaysBind: false,
2530+
participatesInValidation: false,
2531+
isEnabled: () => true,
2532+
getTargetHost: () => 'openrouter.ai',
2533+
getBasePath: () => '',
2534+
getAuthHeaders: () => ({ 'Authorization': 'Bearer sk-or-key' }),
2535+
getBodyTransform: () => null,
2536+
};
2537+
const port = await startAdapter(adapter);
2538+
2539+
await fetch(port, '/v1/chat/completions', { method: 'POST', body: '{}' });
2540+
2541+
jest.restoreAllMocks();
2542+
spy.mockRestore();
2543+
2544+
const authErrLog = lines.find(l => l.event === 'upstream_auth_error' && l.status === 400);
2545+
expect(authErrLog).toBeDefined();
2546+
expect(authErrLog.provider).toBe('copilot');
2547+
expect(authErrLog.message).toContain('400');
2548+
});
2549+
2550+
it('emits upstream_auth_error when upstream returns 401', async () => {
2551+
const { lines, spy } = collectLogOutput();
2552+
mockHttpsWithStatus(401);
2553+
2554+
const adapter = {
2555+
name: 'copilot', port: 0, isManagementPort: false, alwaysBind: false,
2556+
participatesInValidation: false,
2557+
isEnabled: () => true,
2558+
getTargetHost: () => 'api.githubcopilot.com',
2559+
getBasePath: () => '',
2560+
getAuthHeaders: () => ({ 'Authorization': 'Bearer gho_token' }),
2561+
getBodyTransform: () => null,
2562+
};
2563+
const port = await startAdapter(adapter);
2564+
2565+
await fetch(port, '/v1/chat/completions', { method: 'POST', body: '{}' });
2566+
2567+
jest.restoreAllMocks();
2568+
spy.mockRestore();
2569+
2570+
const authErrLog = lines.find(l => l.event === 'upstream_auth_error' && l.status === 401);
2571+
expect(authErrLog).toBeDefined();
2572+
expect(authErrLog.provider).toBe('copilot');
2573+
expect(authErrLog.message).toContain('401');
2574+
});
2575+
2576+
it('does NOT emit upstream_auth_error for a successful 200 response', async () => {
2577+
const { lines, spy } = collectLogOutput();
2578+
mockHttpsWithStatus(200);
2579+
2580+
const adapter = {
2581+
name: 'copilot', port: 0, isManagementPort: false, alwaysBind: false,
2582+
participatesInValidation: false,
2583+
isEnabled: () => true,
2584+
getTargetHost: () => 'api.githubcopilot.com',
2585+
getBasePath: () => '',
2586+
getAuthHeaders: () => ({ 'Authorization': 'Bearer gho_token' }),
2587+
getBodyTransform: () => null,
2588+
};
2589+
const port = await startAdapter(adapter);
2590+
2591+
await fetch(port, '/v1/chat/completions', { method: 'POST', body: '{}' });
2592+
2593+
jest.restoreAllMocks();
2594+
spy.mockRestore();
2595+
2596+
const authErrLog = lines.find(l => l.event === 'upstream_auth_error');
2597+
expect(authErrLog).toBeUndefined();
2598+
});
23652599
});

0 commit comments

Comments
 (0)