refactor: remove duplicate HTTP helpers from OidcTokenProvider#2898
Conversation
Replace private `_httpGet`, `_httpPost`, and `_getProxyAgent` methods in `OidcTokenProvider` with the equivalent exported functions `httpGet` and `httpPost` from `github-oidc.js`. This removes ~65 lines of near-identical production logic in the credential-exchange path and makes `OidcTokenProvider` consistent with the sibling `AwsOidcTokenProvider` and `GcpOidcTokenProvider` classes, which already import from `github-oidc.js`. Also update the integration test to override `_exchangeForAzureToken` (instead of the now-deleted `_httpPost`) to route requests to the local HTTP mock server, matching the pattern used by the AWS and GCP sibling tests.
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Refactors the Azure OidcTokenProvider in containers/api-proxy to stop maintaining its own HTTP/proxy helper implementations and instead reuse the shared github-oidc helpers already used by the AWS/GCP OIDC providers.
Changes:
- Replaced
OidcTokenProvider’s internal HTTP GET/POST + proxy-agent helpers withhttpGet/httpPostfrom./github-oidc. - Deleted the duplicated private helper methods (
_httpGet,_httpPost,_getProxyAgent) fromOidcTokenProvider. - Updated the Azure OIDC provider test to avoid monkey-patching
_httpPost, switching to an_exchangeForAzureTokenoverride that calls sharedhttpPost.
Show a summary per file
| File | Description |
|---|---|
| containers/api-proxy/oidc-token-provider.js | Removes duplicated HTTP/proxy helper logic and uses shared httpGet/httpPost from github-oidc. |
| containers/api-proxy/oidc-token-provider.test.js | Updates the integration-style test strategy to use shared httpPost via an _exchangeForAzureToken override. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
containers/api-proxy/oidc-token-provider.test.js:86
- The overridden
_exchangeForAzureTokenhelper doesn't checkresponse.statusCodebefore JSON-parsing/returning the token. If the mock ever returns a non-200 (or non-JSON) response, the test will fail with a JSON parse error instead of clearly asserting the expected error path. Consider assertingstatusCode === 200(and/or surfacing the response body) to make failures actionable and to align with the production method's behavior.
const response = await httpPost(
`http://127.0.0.1:${serverPort}/test-tenant-id/oauth2/v2.0/token`,
body,
{ 'Content-Type': 'application/x-www-form-urlencoded' }
);
const data = JSON.parse(response.body);
return { access_token: data.access_token, expires_in: data.expires_in || 3600 };
- Files reviewed: 2/2 changed files
- Comments generated: 2
| // Override _exchangeForAzureToken to use mock server over http | ||
| provider._exchangeForAzureToken = async (oidcJwt) => { | ||
| const body = new URLSearchParams({ |
| /** | ||
| * Mint a GitHub OIDC token with the specified audience. | ||
| * @returns {Promise<string>} The GitHub-issued JWT | ||
| */ | ||
| async _mintGitHubOidcToken() { | ||
| const url = new URL(this._requestUrl); | ||
| url.searchParams.set('audience', this._oidcAudience); | ||
|
|
||
| const response = await this._httpGet(url.toString(), { | ||
| const response = await httpGet(url.toString(), { | ||
| 'Authorization': `Bearer ${this._requestToken}`, | ||
| 'Accept': 'application/json', | ||
| }); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@copilot merge main |
…-http-helpers # Conflicts: # containers/api-proxy/oidc-token-provider.js
Done — merged |
🔬 Smoke Test Results
Overall: FAIL The workflow pre-step outputs (
|
Smoke Test Results
Overall: 3/4 tests passed. Core functionality working.
|
Smoke Test: Copilot BYOK (Offline) Mode
Overall: FAIL — pre-computed step outputs were not injected (GitHub Actions expressions not expanded), and GitHub MCP returned 401.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
|
Hide internal CLI exports from the public entry point Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
Chroot Version Comparison Results
Result: Not all tests passed. Python and Node.js versions differ between host and chroot environments.
|
Smoke Test Results
Overall: FAIL — service containers are not reachable via
|
OidcTokenProviderprivately re-implemented_httpGet,_httpPost, and_getProxyAgent— ~65 lines of logic already exported bygithub-oidc.jsand used correctly by the siblingAwsOidcTokenProviderandGcpOidcTokenProvider. The duplicate had already drifted (e.g. timeout message"OIDC request timeout"vs"GitHub OIDC request timeout").✨ Enhancement
containers/api-proxy/oidc-token-provider.jshttps,http,HttpsProxyAgentimportsmintGitHubOidcToken,httpPostfrom./github-oidc_mintGitHubOidcToken()to the sharedmintGitHubOidcToken()utility — consistent withAwsOidcTokenProvider/GcpOidcTokenProvider_httpPostwith a thin wrapper around the sharedhttpPost, preserving test-mockability'Token exchange timeout'errors to the Azure-specific'Azure token exchange timeout'message in_exchangeForAzureToken_httpGet,_getProxyAgentprivate methods (~65 lines total removed)containers/api-proxy/oidc-token-provider.test.js_httpPostto redirect HTTPS→HTTP; replaced with an_exchangeForAzureTokenoverride, matching the pattern used by the AWS and GCP sibling tests._mintGitHubOidcToken()(audience + auth header validation) and_exchangeForAzureToken()timeout error remapping.