diff --git a/containers/api-proxy/providers/copilot.js b/containers/api-proxy/providers/copilot.js index e6590285..a25ed1ad 100644 --- a/containers/api-proxy/providers/copilot.js +++ b/containers/api-proxy/providers/copilot.js @@ -17,17 +17,35 @@ const { normalizeApiTarget } = require('../proxy-utils'); const { URL } = require('url'); +/** + * Strip any accidental "Bearer " prefix from a raw credential value and trim + * surrounding whitespace. Returns undefined when the result is empty so that + * callers can use `|| undefined` fall-through cleanly. + * + * A value like "Bearer " (prefix with nothing after it) reduces to undefined + * rather than "Bearer", which is why the prefix is removed before trimming. + * + * @param {string|undefined} value - Raw credential string + * @returns {string|undefined} + */ +function stripBearerPrefix(value) { + return ((value || '').replace(/^\s*Bearer\s+/i, '').trim()) || undefined; +} + /** * Resolves the Copilot auth token from environment variables. * COPILOT_GITHUB_TOKEN (GitHub OAuth) takes precedence over COPILOT_API_KEY (direct key). * + * Any accidental "Bearer " prefix is stripped via stripBearerPrefix so that + * the injected Authorization header is exactly "Bearer " rather than + * the double-prefixed "Bearer Bearer " that would be rejected by + * external providers in BYOK mode. + * * @param {Record} env - Environment variables to inspect * @returns {string|undefined} The resolved auth token, or undefined if neither is set */ function resolveCopilotAuthToken(env = process.env) { - const githubToken = (env.COPILOT_GITHUB_TOKEN || '').trim() || undefined; - const apiKey = (env.COPILOT_API_KEY || '').trim() || undefined; - return githubToken || apiKey; + return stripBearerPrefix(env.COPILOT_GITHUB_TOKEN) || stripBearerPrefix(env.COPILOT_API_KEY); } /** @@ -127,8 +145,8 @@ function deriveGitHubApiBasePath(env = process.env) { * @returns {import('./index').ProviderAdapter} */ function createCopilotAdapter(env, deps = {}) { - const githubToken = (env.COPILOT_GITHUB_TOKEN || '').trim() || undefined; - const apiKey = (env.COPILOT_API_KEY || '').trim() || undefined; + const githubToken = stripBearerPrefix(env.COPILOT_GITHUB_TOKEN); + const apiKey = stripBearerPrefix(env.COPILOT_API_KEY); const authToken = resolveCopilotAuthToken(env); const integrationId = env.COPILOT_INTEGRATION_ID || 'copilot-developer-cli'; const rawTarget = deriveCopilotApiTarget(env); @@ -245,6 +263,7 @@ function createCopilotAdapter(env, deps = {}) { module.exports = { createCopilotAdapter, resolveCopilotAuthToken, + stripBearerPrefix, deriveCopilotApiTarget, deriveGitHubApiTarget, deriveGitHubApiBasePath, diff --git a/containers/api-proxy/server.js b/containers/api-proxy/server.js index 1f8b6fce..d7a3e169 100644 --- a/containers/api-proxy/server.js +++ b/containers/api-proxy/server.js @@ -379,11 +379,11 @@ function proxyRequest(req, res, targetHost, injectHeaders, provider, basePath = const resHeaders = { ...proxyRes.headers, 'x-request-id': requestId }; - if (proxyRes.statusCode === 401 || proxyRes.statusCode === 403) { + if (proxyRes.statusCode === 400 || proxyRes.statusCode === 401 || proxyRes.statusCode === 403) { logRequest('warn', 'upstream_auth_error', { request_id: requestId, provider, status: proxyRes.statusCode, upstream_host: targetHost, path: sanitizeForLog(req.url), - message: `Upstream returned ${proxyRes.statusCode} — check that the API key is valid and has not expired`, + message: `Upstream returned ${proxyRes.statusCode} — check that the API key is valid and correctly formatted`, }); } diff --git a/containers/api-proxy/server.test.js b/containers/api-proxy/server.test.js index 605e9738..69eb1f5e 100644 --- a/containers/api-proxy/server.test.js +++ b/containers/api-proxy/server.test.js @@ -11,7 +11,7 @@ const { EventEmitter } = require('events'); const { normalizeApiTarget, normalizeBasePath, buildUpstreamPath, shouldStripHeader, stripGeminiKeyParam, composeBodyTransforms } = require('./proxy-utils'); // Provider-specific functions that live in their respective adapter modules -const { deriveCopilotApiTarget, deriveGitHubApiTarget, deriveGitHubApiBasePath, resolveCopilotAuthToken } = require('./providers/copilot'); +const { deriveCopilotApiTarget, deriveGitHubApiTarget, deriveGitHubApiBasePath, resolveCopilotAuthToken, stripBearerPrefix, createCopilotAdapter } = require('./providers/copilot'); const { resolveOpenCodeRoute } = require('./providers/opencode'); // Core proxy functions that remain in server.js @@ -883,6 +883,45 @@ describe('proxyWebSocket', () => { }); }); +describe('stripBearerPrefix', () => { + it('strips "Bearer " prefix from a token value', () => { + expect(stripBearerPrefix('Bearer sk-or-v1-abc')).toBe('sk-or-v1-abc'); + }); + + it('strips "Bearer " prefix case-insensitively', () => { + expect(stripBearerPrefix('bearer sk-or-v1-abc')).toBe('sk-or-v1-abc'); + expect(stripBearerPrefix('BEARER sk-or-v1-abc')).toBe('sk-or-v1-abc'); + }); + + it('strips leading whitespace before "Bearer "', () => { + expect(stripBearerPrefix(' Bearer sk-or-v1-abc')).toBe('sk-or-v1-abc'); + }); + + it('returns value unchanged when no "Bearer " prefix is present', () => { + expect(stripBearerPrefix('sk-or-v1-abc')).toBe('sk-or-v1-abc'); + expect(stripBearerPrefix('gho_abc123')).toBe('gho_abc123'); + }); + + it('does not strip "Bearer" without a following space', () => { + expect(stripBearerPrefix('BearerToken123')).toBe('BearerToken123'); + }); + + it('returns undefined when value is only "Bearer " (nothing after prefix)', () => { + expect(stripBearerPrefix('Bearer ')).toBeUndefined(); + expect(stripBearerPrefix('Bearer ')).toBeUndefined(); + }); + + it('returns undefined for empty or whitespace-only input', () => { + expect(stripBearerPrefix('')).toBeUndefined(); + expect(stripBearerPrefix(' ')).toBeUndefined(); + expect(stripBearerPrefix(undefined)).toBeUndefined(); + }); + + it('trims surrounding whitespace from the token', () => { + expect(stripBearerPrefix(' sk-or-v1-abc ')).toBe('sk-or-v1-abc'); + }); +}); + describe('resolveCopilotAuthToken', () => { it('should return COPILOT_GITHUB_TOKEN when only it is set', () => { expect(resolveCopilotAuthToken({ COPILOT_GITHUB_TOKEN: 'gho_abc123' })).toBe('gho_abc123'); @@ -921,6 +960,94 @@ describe('resolveCopilotAuthToken', () => { COPILOT_API_KEY: 'sk-byok-key', })).toBe('sk-byok-key'); }); + + // Integration: verify that Bearer-prefix stripping (via stripBearerPrefix) is + // applied to both token sources when resolving. + + it('strips "Bearer " prefix from COPILOT_API_KEY when resolving', () => { + expect(resolveCopilotAuthToken({ COPILOT_API_KEY: 'Bearer sk-or-v1-abc' })).toBe('sk-or-v1-abc'); + }); + + it('strips "Bearer " prefix from COPILOT_GITHUB_TOKEN when resolving', () => { + expect(resolveCopilotAuthToken({ COPILOT_GITHUB_TOKEN: 'Bearer gho_abc123' })).toBe('gho_abc123'); + }); + + it('prefers stripped COPILOT_GITHUB_TOKEN over stripped COPILOT_API_KEY', () => { + expect(resolveCopilotAuthToken({ + COPILOT_GITHUB_TOKEN: 'Bearer gho_abc123', + COPILOT_API_KEY: 'Bearer sk-byok-key', + })).toBe('gho_abc123'); + }); +}); + +// ── createCopilotAdapter — BYOK auth header format ─────────────────────────── +// +// These tests guard against the "badly formatted Authorization header" bug in +// BYOK mode where the sidecar is configured with COPILOT_API_KEY (the real key +// held by the sidecar) and could produce "Authorization: Bearer Bearer " +// if the COPILOT_API_KEY value already contained the "Bearer " prefix. +// They also verify that the header injected for inference requests is exactly +// "Bearer " and that the Copilot-Integration-Id header is present. + +describe('createCopilotAdapter — BYOK getAuthHeaders', () => { + const fakeReq = { url: '/v1/chat/completions', method: 'POST', headers: {} }; + const fakeModelsReq = { url: '/models', method: 'GET', headers: {} }; + + it('injects Authorization: Bearer for BYOK inference request', () => { + const adapter = createCopilotAdapter({ COPILOT_API_KEY: 'sk-or-v1-abc123' }); + const headers = adapter.getAuthHeaders(fakeReq); + expect(headers['Authorization']).toBe('Bearer sk-or-v1-abc123'); + }); + + it('injects Copilot-Integration-Id header for BYOK inference request', () => { + const adapter = createCopilotAdapter({ COPILOT_API_KEY: 'sk-or-v1-abc123' }); + const headers = adapter.getAuthHeaders(fakeReq); + expect(headers['Copilot-Integration-Id']).toBe('copilot-developer-cli'); + }); + + it('prevents double "Bearer " prefix when API key already contains "Bearer " prefix (BYOK bug fix)', () => { + const adapter = createCopilotAdapter({ COPILOT_API_KEY: 'Bearer sk-or-v1-abc123' }); + const headers = adapter.getAuthHeaders(fakeReq); + // Must NOT be "Bearer Bearer sk-or-v1-abc123" + expect(headers['Authorization']).toBe('Bearer sk-or-v1-abc123'); + expect(headers['Authorization']).not.toContain('Bearer Bearer'); + }); + + it('strips "Bearer " prefix case-insensitively from API key', () => { + const adapter = createCopilotAdapter({ COPILOT_API_KEY: 'BEARER sk-or-v1-abc123' }); + const headers = adapter.getAuthHeaders(fakeReq); + expect(headers['Authorization']).toBe('Bearer sk-or-v1-abc123'); + }); + + it('uses COPILOT_GITHUB_TOKEN (not COPILOT_API_KEY) for /models GET in BYOK+token mode', () => { + const adapter = createCopilotAdapter({ + COPILOT_GITHUB_TOKEN: 'gho_oauth_token', + COPILOT_API_KEY: 'sk-or-v1-abc123', + }); + const headers = adapter.getAuthHeaders(fakeModelsReq); + expect(headers['Authorization']).toBe('Bearer gho_oauth_token'); + }); + + it('uses API key for /models GET when no GITHUB_TOKEN is set (BYOK-only mode)', () => { + const adapter = createCopilotAdapter({ COPILOT_API_KEY: 'sk-or-v1-abc123' }); + const headers = adapter.getAuthHeaders(fakeModelsReq); + // In BYOK-only mode, githubToken is undefined so falls through to authToken (apiKey) + expect(headers['Authorization']).toBe('Bearer sk-or-v1-abc123'); + }); + + it('is enabled when only COPILOT_API_KEY is set', () => { + const adapter = createCopilotAdapter({ COPILOT_API_KEY: 'sk-or-v1-abc123' }); + expect(adapter.isEnabled()).toBe(true); + }); + + it('uses custom COPILOT_INTEGRATION_ID when set', () => { + const adapter = createCopilotAdapter({ + COPILOT_API_KEY: 'sk-or-v1-abc123', + COPILOT_INTEGRATION_ID: 'my-custom-integration', + }); + const headers = adapter.getAuthHeaders(fakeReq); + expect(headers['Copilot-Integration-Id']).toBe('my-custom-integration'); + }); }); describe('resolveOpenCodeRoute', () => { @@ -2362,4 +2489,111 @@ describe('createProviderServer', () => { await new Promise((r) => setTimeout(r, 100)); expect(callCount).toBe(1); }); + + // ── 400/401/403 upstream response → upstream_auth_error log ────────────── + // + // When the upstream provider returns an auth-related error status, the proxy + // must emit an 'upstream_auth_error' log event so operators can diagnose + // credential problems quickly. A 400 specifically indicates a possible + // malformed Authorization header (e.g. double "Bearer " prefix in BYOK mode). + + /** + * Build a minimal mock for https.request that immediately calls back with a + * response of the given status code. The mock proxyRes emits 'end' after + * the callback so request_complete is also logged. + */ + function mockHttpsWithStatus(statusCode) { + return jest.spyOn(https, 'request').mockImplementation((options, callback) => { + const proxyReq = new EventEmitter(); + proxyReq.write = jest.fn(); + proxyReq.end = jest.fn(() => { + setImmediate(() => { + const proxyRes = new EventEmitter(); + proxyRes.statusCode = statusCode; + proxyRes.headers = { 'content-type': 'application/json' }; + proxyRes.pipe = jest.fn((destRes) => { destRes.end('{}'); }); + callback(proxyRes); + setImmediate(() => proxyRes.emit('end')); + }); + }); + proxyReq.destroy = jest.fn(); + return proxyReq; + }); + } + + it('emits upstream_auth_error when upstream returns 400', async () => { + const { lines, spy } = collectLogOutput(); + mockHttpsWithStatus(400); + + const adapter = { + name: 'copilot', port: 0, isManagementPort: false, alwaysBind: false, + participatesInValidation: false, + isEnabled: () => true, + getTargetHost: () => 'openrouter.ai', + getBasePath: () => '', + getAuthHeaders: () => ({ 'Authorization': 'Bearer sk-or-key' }), + getBodyTransform: () => null, + }; + const port = await startAdapter(adapter); + + await fetch(port, '/v1/chat/completions', { method: 'POST', body: '{}' }); + + jest.restoreAllMocks(); + spy.mockRestore(); + + const authErrLog = lines.find(l => l.event === 'upstream_auth_error' && l.status === 400); + expect(authErrLog).toBeDefined(); + expect(authErrLog.provider).toBe('copilot'); + expect(authErrLog.message).toContain('400'); + }); + + it('emits upstream_auth_error when upstream returns 401', async () => { + const { lines, spy } = collectLogOutput(); + mockHttpsWithStatus(401); + + const adapter = { + name: 'copilot', port: 0, isManagementPort: false, alwaysBind: false, + participatesInValidation: false, + isEnabled: () => true, + getTargetHost: () => 'api.githubcopilot.com', + getBasePath: () => '', + getAuthHeaders: () => ({ 'Authorization': 'Bearer gho_token' }), + getBodyTransform: () => null, + }; + const port = await startAdapter(adapter); + + await fetch(port, '/v1/chat/completions', { method: 'POST', body: '{}' }); + + jest.restoreAllMocks(); + spy.mockRestore(); + + const authErrLog = lines.find(l => l.event === 'upstream_auth_error' && l.status === 401); + expect(authErrLog).toBeDefined(); + expect(authErrLog.provider).toBe('copilot'); + expect(authErrLog.message).toContain('401'); + }); + + it('does NOT emit upstream_auth_error for a successful 200 response', async () => { + const { lines, spy } = collectLogOutput(); + mockHttpsWithStatus(200); + + const adapter = { + name: 'copilot', port: 0, isManagementPort: false, alwaysBind: false, + participatesInValidation: false, + isEnabled: () => true, + getTargetHost: () => 'api.githubcopilot.com', + getBasePath: () => '', + getAuthHeaders: () => ({ 'Authorization': 'Bearer gho_token' }), + getBodyTransform: () => null, + }; + const port = await startAdapter(adapter); + + await fetch(port, '/v1/chat/completions', { method: 'POST', body: '{}' }); + + jest.restoreAllMocks(); + spy.mockRestore(); + + const authErrLog = lines.find(l => l.event === 'upstream_auth_error'); + expect(authErrLog).toBeUndefined(); + }); });