fix(api-proxy): address OIDC review feedback

Copilot · web-flow · commit 8ce7a7d75aaa · 2026-05-05T23:04:00.000Z
Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/074a922b-ae9a-4c09-a168-6379e90be199
diff --git a/containers/api-proxy/oidc-token-provider.js b/containers/api-proxy/oidc-token-provider.js
@@ -16,6 +16,7 @@
 
 const https = require('https');
 const http = require('http');
+const { HttpsProxyAgent } = require('https-proxy-agent');
 const { logRequest } = require('./logging');
 
 // Refresh at 75% of token lifetime (Azure tokens typically last 3600s)
@@ -219,8 +220,11 @@ class OidcTokenProvider {
 
     // Schedule proactive refresh
     const refreshInSecs = Math.max(
+      0,
+      Math.min(
       expires_in * REFRESH_FACTOR,
       expires_in - MIN_REFRESH_MARGIN_SECS
+      )
     );
     this._scheduleRefresh(Math.floor(refreshInSecs * 1000));
   }
@@ -262,7 +266,10 @@ class OidcTokenProvider {
     return new Promise((resolve, reject) => {
       const parsedUrl = new URL(url);
       const mod = parsedUrl.protocol === 'https:' ? https : http;
-      const req = mod.get(url, { headers }, (res) => {
+      const req = mod.get(url, {
+        headers,
+        agent: this._getProxyAgent(parsedUrl),
+      }, (res) => {
         let body = '';
         res.on('data', (chunk) => { body += chunk; });
         res.on('end', () => resolve({ statusCode: res.statusCode, body }));
@@ -285,60 +292,37 @@ class OidcTokenProvider {
       const options = {
         method: 'POST',
         hostname: parsedUrl.hostname,
-        port: parsedUrl.port || 443,
+        port: parsedUrl.port || (parsedUrl.protocol === 'https:' ? 443 : 80),
         path: parsedUrl.pathname + parsedUrl.search,
         headers: { ...headers, 'Content-Length': Buffer.byteLength(body) },
+        agent: this._getProxyAgent(parsedUrl),
       };
 
-      // Use HTTP_PROXY if set (sidecar routes through Squid)
-      const proxyUrl = process.env.HTTP_PROXY || process.env.HTTPS_PROXY;
-      let req;
-      if (proxyUrl && parsedUrl.protocol === 'https:') {
-        // For HTTPS through proxy, use CONNECT method via http module
-        const proxy = new URL(proxyUrl);
-        const connectReq = http.request({
-          host: proxy.hostname,
-          port: proxy.port || 3128,
-          method: 'CONNECT',
-          path: `${parsedUrl.hostname}:443`,
-        });
-        connectReq.on('connect', (connectRes, socket) => {
-          if (connectRes.statusCode !== 200) {
-            reject(new Error(`Proxy CONNECT failed: ${connectRes.statusCode}`));
-            return;
-          }
-          req = https.request({
-            ...options,
-            socket,
-            agent: false,
-          }, (res) => {
-            let responseBody = '';
-            res.on('data', (chunk) => { responseBody += chunk; });
-            res.on('end', () => resolve({ statusCode: res.statusCode, body: responseBody }));
-          });
-          req.on('error', reject);
-          req.setTimeout(10_000, () => { req.destroy(new Error('Azure token exchange timeout')); });
-          req.write(body);
-          req.end();
-        });
-        connectReq.on('error', reject);
-        connectReq.setTimeout(10_000, () => { connectReq.destroy(new Error('Proxy connect timeout')); });
-        connectReq.end();
-      } else {
-        const mod = parsedUrl.protocol === 'https:' ? https : http;
-        req = mod.request(options, (res) => {
-          let responseBody = '';
-          res.on('data', (chunk) => { responseBody += chunk; });
-          res.on('end', () => resolve({ statusCode: res.statusCode, body: responseBody }));
-        });
-        req.on('error', reject);
-        req.setTimeout(10_000, () => { req.destroy(new Error('Azure token exchange timeout')); });
-        req.write(body);
-        req.end();
-      }
+      const mod = parsedUrl.protocol === 'https:' ? https : http;
+      const req = mod.request(options, (res) => {
+        let responseBody = '';
+        res.on('data', (chunk) => { responseBody += chunk; });
+        res.on('end', () => resolve({ statusCode: res.statusCode, body: responseBody }));
+      });
+      req.on('error', reject);
+      req.setTimeout(10_000, () => { req.destroy(new Error('Azure token exchange timeout')); });
+      req.write(body);
+      req.end();
     });
   }
 
+  /**
+   * Build proxy agent from env vars when configured.
+   * @param {URL} parsedUrl
+   * @returns {import('http').Agent|undefined}
+   */
+  _getProxyAgent(parsedUrl) {
+    const proxyUrl = parsedUrl.protocol === 'https:'
+      ? (process.env.HTTPS_PROXY || process.env.HTTP_PROXY)
+      : (process.env.HTTP_PROXY || process.env.HTTPS_PROXY);
+    return proxyUrl ? new HttpsProxyAgent(proxyUrl) : undefined;
+  }
+
   /** @param {number} ms */
   _sleep(ms) {
     return new Promise(resolve => setTimeout(resolve, ms));
diff --git a/containers/api-proxy/oidc-token-provider.test.js b/containers/api-proxy/oidc-token-provider.test.js
@@ -70,7 +70,6 @@ describe('OidcTokenProvider', () => {
     // Override login host to use mock server
     provider._loginHost = `127.0.0.1:${serverPort}`;
     // Override _httpPost to use http (not https)
-    const originalPost = provider._httpPost.bind(provider);
     provider._httpPost = function (url, body, headers) {
       // Rewrite https to http for mock
       const httpUrl = url.replace('https://', 'http://');
@@ -173,12 +172,54 @@ describe('OidcTokenProvider', () => {
     provider.shutdown();
     await new Promise(resolve => failServer.close(resolve));
   });
+
+  it('should schedule refresh at 75% or 5 minutes-before-expiry, whichever is earlier', async () => {
+    const provider = new OidcTokenProvider({
+      requestUrl: 'http://localhost/token',
+      requestToken: 'test',
+      tenantId: 'test',
+      clientId: 'test',
+    });
+
+    provider._mintGitHubOidcToken = jest.fn().mockResolvedValue('oidc-jwt');
+    provider._exchangeForAzureToken = jest.fn().mockResolvedValue({
+      access_token: 'azure-token',
+      expires_in: 600,
+    });
+    provider._scheduleRefresh = jest.fn();
+
+    await provider._refreshToken();
+
+    expect(provider._scheduleRefresh).toHaveBeenCalledWith(300000);
+    provider.shutdown();
+  });
+
+  it('should schedule immediate refresh when token lifetime is below minimum margin', async () => {
+    const provider = new OidcTokenProvider({
+      requestUrl: 'http://localhost/token',
+      requestToken: 'test',
+      tenantId: 'test',
+      clientId: 'test',
+    });
+
+    provider._mintGitHubOidcToken = jest.fn().mockResolvedValue('oidc-jwt');
+    provider._exchangeForAzureToken = jest.fn().mockResolvedValue({
+      access_token: 'azure-token',
+      expires_in: 240,
+    });
+    provider._scheduleRefresh = jest.fn();
+
+    await provider._refreshToken();
+
+    expect(provider._scheduleRefresh).toHaveBeenCalledWith(0);
+    provider.shutdown();
+  });
 });
 
 describe('OpenAI adapter with OIDC', () => {
   const { createOpenAIAdapter } = require('./providers/openai');
 
-  it('should report enabled when OIDC is configured', () => {
+  it('should report disabled until OIDC token is initialized', () => {
     const adapter = createOpenAIAdapter({
       AWF_AUTH_TYPE: 'github-oidc',
       ACTIONS_ID_TOKEN_REQUEST_URL: 'http://localhost/token',
@@ -188,7 +229,7 @@ describe('OpenAI adapter with OIDC', () => {
       OPENAI_API_TARGET: 'my-resource.openai.azure.com',
     });
 
-    expect(adapter.isEnabled()).toBe(true);
+    expect(adapter.isEnabled()).toBe(false);
     expect(adapter.getOidcProvider()).not.toBeNull();
     expect(adapter.getValidationProbe()).toEqual({ skip: true, reason: 'OIDC auth; validation via token acquisition' });
     expect(adapter.getModelsFetchConfig()).toBeNull();
@@ -217,7 +258,7 @@ describe('OpenAI adapter with OIDC', () => {
     expect(adapter.getOidcProvider()).toBeNull();
   });
 
-  it('should return oidc-token-unavailable when OIDC token not yet acquired', () => {
+  it('should return empty auth headers when OIDC token is not yet acquired', () => {
     const adapter = createOpenAIAdapter({
       AWF_AUTH_TYPE: 'github-oidc',
       ACTIONS_ID_TOKEN_REQUEST_URL: 'http://localhost/token',
@@ -228,7 +269,27 @@ describe('OpenAI adapter with OIDC', () => {
 
     // Before initialization, token should be unavailable
     const headers = adapter.getAuthHeaders({});
-    expect(headers['Authorization']).toBe('Bearer oidc-token-unavailable');
+    expect(headers).toEqual({});
+
+    adapter.getOidcProvider().shutdown();
+  });
+
+  it('should inject only Authorization header in OIDC mode', () => {
+    const adapter = createOpenAIAdapter({
+      AWF_AUTH_TYPE: 'github-oidc',
+      ACTIONS_ID_TOKEN_REQUEST_URL: 'http://localhost/token',
+      ACTIONS_ID_TOKEN_REQUEST_TOKEN: 'test-token',
+      AWF_AUTH_AZURE_TENANT_ID: 'test-tenant',
+      AWF_AUTH_AZURE_CLIENT_ID: 'test-client',
+    });
+
+    const provider = adapter.getOidcProvider();
+    provider._cachedToken = 'azure-ad-token';
+    provider._expiresAt = Math.floor(Date.now() / 1000) + 600;
+
+    const headers = adapter.getAuthHeaders({});
+    expect(headers).toEqual({ Authorization: 'Bearer azure-ad-token' });
+    expect(headers['api-key']).toBeUndefined();
 
     adapter.getOidcProvider().shutdown();
   });
diff --git a/containers/api-proxy/providers/openai.js b/containers/api-proxy/providers/openai.js
@@ -56,7 +56,7 @@ function createOpenAIAdapter(env, deps = {}) {
       });
     }
   }
-  const oidcEnabled = !!oidcProvider;
+  const oidcConfigured = !!oidcProvider;
 
   return {
     name: 'openai',
@@ -74,7 +74,7 @@ function createOpenAIAdapter(env, deps = {}) {
     /** Port 10000 always counts toward the startup validation latch. */
     participatesInValidation: true,
 
-    isEnabled() { return !!apiKey || oidcEnabled; },
+    isEnabled() { return !!apiKey || !!oidcProvider?.isReady(); },
     getTargetHost() { return rawTarget; },
     getBasePath() { return basePath; },
 
@@ -90,11 +90,9 @@ function createOpenAIAdapter(env, deps = {}) {
       if (oidcProvider) {
         const token = oidcProvider.getToken();
         if (token) {
-          return { 'Authorization': `Bearer ${token}`, 'api-key': token };
+          return { 'Authorization': `Bearer ${token}` };
         }
-        // Token not yet available (pre-init or refresh failure)
-        // Return empty — server will return 503 via isEnabled() short-circuit
-        return { 'Authorization': 'Bearer oidc-token-unavailable' };
+        return {};
       }
       return { 'Authorization': `Bearer ${apiKey}` };
     },
@@ -109,7 +107,7 @@ function createOpenAIAdapter(env, deps = {}) {
      * @returns {{ url: string, opts: object }|{ skip: true, reason: string }|null}
      */
     getValidationProbe() {
-      if (oidcEnabled) {
+      if (oidcConfigured) {
         return { skip: true, reason: 'OIDC auth; validation via token acquisition' };
       }
       if (!apiKey) return null;
@@ -130,7 +128,7 @@ function createOpenAIAdapter(env, deps = {}) {
      * @returns {{ url: string, opts: object, cacheKey: string }|null}
      */
     getModelsFetchConfig() {
-      if (oidcEnabled) return null; // Models fetched after OIDC init
+      if (oidcConfigured) return null; // Models fetched after OIDC init
       if (!apiKey) return null;
       const modelsPath = basePath ? `${basePath}/models` : '/v1/models';
       return {
@@ -145,15 +143,21 @@ function createOpenAIAdapter(env, deps = {}) {
         provider: 'openai',
         port: 10000,
         base_url: 'http://api-proxy:10000',
-        configured: !!apiKey || oidcEnabled,
-        auth_type: oidcEnabled ? 'github-oidc' : 'static-key',
+        configured: !!apiKey || oidcConfigured,
+        auth_type: oidcConfigured ? 'github-oidc' : 'static-key',
         models_cache_key: 'openai',
         models_url: 'http://api-proxy:10000/v1/models',
       };
     },
 
     /** Response returned when port 10000 receives a proxy request but no key is set. */
     getUnconfiguredResponse() {
+      if (oidcConfigured) {
+        return {
+          statusCode: 503,
+          body: { error: 'OpenAI OIDC token unavailable; retry shortly' },
+        };
+      }
       return {
         statusCode: 404,
         body: { error: 'OpenAI proxy not configured (no OPENAI_API_KEY or OIDC auth)' },
diff --git a/src/services/api-proxy-service.test.ts b/src/services/api-proxy-service.test.ts
@@ -492,6 +492,55 @@ describe('API proxy sidecar', () => {
         expect(env.AWF_ENABLE_OPENCODE).toBeUndefined();
       });
 
+      describe('OIDC runtime env forwarding', () => {
+        let savedEnv: Record<string, string | undefined>;
+        const oidcVars = [
+          'AWF_AUTH_TYPE',
+          'ACTIONS_ID_TOKEN_REQUEST_URL',
+          'ACTIONS_ID_TOKEN_REQUEST_TOKEN',
+        ];
+
+        beforeEach(() => {
+          savedEnv = {};
+          for (const key of oidcVars) {
+            savedEnv[key] = process.env[key];
+            delete process.env[key];
+          }
+        });
+
+        afterEach(() => {
+          for (const key of oidcVars) {
+            if (savedEnv[key] !== undefined) {
+              process.env[key] = savedEnv[key];
+            } else {
+              delete process.env[key];
+            }
+          }
+        });
+
+        it('should forward ACTIONS_ID_TOKEN_REQUEST_* when AWF_AUTH_TYPE normalizes to github-oidc', () => {
+          process.env.AWF_AUTH_TYPE = '  GitHub-OIDC ';
+          process.env.ACTIONS_ID_TOKEN_REQUEST_URL = 'https://actions.local/token';
+          process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN = 'runtime-token';
+          const config = { ...mockConfig, enableApiProxy: true, openaiApiKey: 'sk-openai-test' };
+          const result = generateDockerCompose(config, mockNetworkConfigWithProxy);
+          const env = result.services['api-proxy'].environment as Record<string, string>;
+          expect(env.ACTIONS_ID_TOKEN_REQUEST_URL).toBe('https://actions.local/token');
+          expect(env.ACTIONS_ID_TOKEN_REQUEST_TOKEN).toBe('runtime-token');
+        });
+
+        it('should not forward ACTIONS_ID_TOKEN_REQUEST_* when AWF_AUTH_TYPE is not github-oidc', () => {
+          process.env.AWF_AUTH_TYPE = 'api-key';
+          process.env.ACTIONS_ID_TOKEN_REQUEST_URL = 'https://actions.local/token';
+          process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN = 'runtime-token';
+          const config = { ...mockConfig, enableApiProxy: true, openaiApiKey: 'sk-openai-test' };
+          const result = generateDockerCompose(config, mockNetworkConfigWithProxy);
+          const env = result.services['api-proxy'].environment as Record<string, string>;
+          expect(env.ACTIONS_ID_TOKEN_REQUEST_URL).toBeUndefined();
+          expect(env.ACTIONS_ID_TOKEN_REQUEST_TOKEN).toBeUndefined();
+        });
+      });
+
       describe('AWF_ANTHROPIC_* env var forwarding', () => {
         let savedEnv: Record<string, string | undefined>;
         const anthropicVars = [
diff --git a/src/services/api-proxy-service.ts b/src/services/api-proxy-service.ts
@@ -34,6 +34,7 @@ export interface ApiProxyServiceParams {
 export function buildApiProxyService(params: ApiProxyServiceParams): ApiProxyBuildResult {
   const { config, networkConfig, apiProxyLogsPath, imageConfig } = params;
   const { useGHCR, registry, parsedTag, projectRoot } = imageConfig;
+  const normalizedAuthType = (process.env.AWF_AUTH_TYPE || '').trim().toLowerCase();
 
   if (!networkConfig.proxyIp) {
     throw new Error('buildApiProxyService: networkConfig.proxyIp is required');
@@ -111,10 +112,10 @@ export function buildApiProxyService(params: ApiProxyServiceParams): ApiProxyBui
       ...(process.env.AWF_AUTH_AZURE_SCOPE && { AWF_AUTH_AZURE_SCOPE: process.env.AWF_AUTH_AZURE_SCOPE }),
       ...(process.env.AWF_AUTH_AZURE_CLOUD && { AWF_AUTH_AZURE_CLOUD: process.env.AWF_AUTH_AZURE_CLOUD }),
       // GitHub Actions OIDC runtime tokens (needed by OIDC token provider in api-proxy)
-      ...(process.env.AWF_AUTH_TYPE === 'github-oidc' && process.env.ACTIONS_ID_TOKEN_REQUEST_URL && {
+      ...(normalizedAuthType === 'github-oidc' && process.env.ACTIONS_ID_TOKEN_REQUEST_URL && {
         ACTIONS_ID_TOKEN_REQUEST_URL: process.env.ACTIONS_ID_TOKEN_REQUEST_URL,
       }),
-      ...(process.env.AWF_AUTH_TYPE === 'github-oidc' && process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN && {
+      ...(normalizedAuthType === 'github-oidc' && process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN && {
         ACTIONS_ID_TOKEN_REQUEST_TOKEN: process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN,
       }),
       // Anthropic request optimisations (all opt-in via env vars on the host)
