fix: normalize API target env vars to bare hostnames via URL parsing#1799
fix: normalize API target env vars to bare hostnames via URL parsing#1799
Conversation
When --*-api-target values originate from GitHub Actions expressions
(e.g., ${{ vars.ANTHROPIC_BASE_URL }}), the scheme is not stripped at
compile time. At runtime the full URL (https://host) reaches the API
proxy, which prepends https:// again, producing double-scheme URLs
like https://https://host that Squid rejects.
Fix at two layers:
1. containers/api-proxy/server.js: add normalizeApiTarget() that
strips any http(s):// prefix. Applied to all four API targets
(OpenAI, Anthropic, Gemini, Copilot) on startup.
2. src/docker-manager.ts: add stripScheme() and apply it when
setting *_API_TARGET env vars in the api-proxy container. This
prevents the scheme from ever reaching the container.
Fixes #1795
Upstream: github/gh-aw#25137
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
Fixes a runtime “double scheme” bug in the API proxy by normalizing scheme-prefixed *_API_TARGET values (e.g., https://...) that can arrive via GitHub Actions expression resolution.
Changes:
- Add runtime API target normalization in
containers/api-proxy/server.jsand export it for tests. - Strip scheme prefixes when generating the api-proxy container environment in
generateDockerCompose(). - Add unit/integration tests covering scheme stripping behavior and a regression demonstration for the double-scheme parsing issue.
Show a summary per file
| File | Description |
|---|---|
containers/api-proxy/server.js |
Adds normalizeApiTarget() and applies it to provider target env vars. |
containers/api-proxy/server.test.js |
Adds tests for normalizeApiTarget() and a regression demonstration around URL parsing. |
src/docker-manager.ts |
Adds stripScheme() and applies it when setting api-proxy *_API_TARGET env vars. |
src/docker-manager.test.ts |
Adds unit tests for stripScheme() and a compose env integration test for scheme stripping. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
containers/api-proxy/server.js:80
- The JSDoc for
normalizeApiTarget()says it returnsundefinedwhen the input is falsy, but the implementation returns the original falsy value (e.g.''stays''). Either update the JSDoc to match the behavior or adjust the function to returnundefinedfor empty/whitespace-only strings so callers have consistent semantics.
* @param {string|undefined} value - Raw env var value
* @returns {string|undefined} Bare hostname, or undefined if input is falsy
*/
function normalizeApiTarget(value) {
if (!value) return value;
return value.trim().replace(/^https?:\/\//, '');
}
containers/api-proxy/server.test.js:34
- Test name is misleading:
normalizeApiTarget('')is asserted to return'', notundefined. Either adjust the expectation (if you want empty string to normalize toundefined) or rename the test to reflect the actual intended behavior.
it('should return undefined for falsy input', () => {
expect(normalizeApiTarget(undefined)).toBeUndefined();
expect(normalizeApiTarget('')).toBe('');
});
- Files reviewed: 4/4 changed files
- Comments generated: 3
| /** | ||
| * Normalizes an API target value to a bare hostname (+ optional path). | ||
| * Strips any https?:// scheme prefix that may arrive when the value | ||
| * originates from a GitHub Actions expression resolved at runtime | ||
| * (e.g., ${{ vars.ANTHROPIC_BASE_URL }} → "https://my-gateway.example.com"). | ||
| * Also trims whitespace for safety. | ||
| * | ||
| * @param {string|undefined} value - Raw env var value | ||
| * @returns {string|undefined} Bare hostname, or undefined if input is falsy | ||
| */ | ||
| function normalizeApiTarget(value) { | ||
| if (!value) return value; | ||
| return value.trim().replace(/^https?:\/\//, ''); | ||
| } |
There was a problem hiding this comment.
normalizeApiTarget() doc/comment says the target may include an optional path, and the tests assert paths are preserved. But targetHost is later used as https.request({ hostname: targetHost }) and as tls.connect({ servername: targetHost }), so any /path (or ?query, #fragment) will produce an invalid hostname/SNI and break requests. Consider parsing the value as a URL/host and returning only the hostname (and possibly rejecting/warning if a path is present), keeping path configuration solely in the existing *_API_BASE_PATH env vars.
This issue also appears on line 74 of the same file.
See below for a potential fix:
* Normalizes an API target value to a bare hostname.
* Accepts either a hostname or a full URL and extracts only the hostname,
* discarding any scheme, path, query, fragment, credentials, or port.
* Path configuration must be provided separately via the existing
* *_API_BASE_PATH environment variables.
*
* @param {string|undefined} value - Raw env var value
* @returns {string|undefined} Bare hostname, or undefined if input is falsy
*/
function normalizeApiTarget(value) {
if (!value) return value;
const trimmed = value.trim();
if (!trimmed) return undefined;
const candidate = /^[a-zA-Z][a-zA-Z\d+\-.]*:\/\//.test(trimmed)
? trimmed
: `https://${trimmed}`;
try {
const parsed = new URL(candidate);
if (parsed.pathname !== '/' || parsed.search || parsed.hash || parsed.username || parsed.password || parsed.port) {
console.warn(
`Ignoring unsupported API target URL components in ${sanitizeForLog(trimmed)}; ` +
'configure path prefixes via the corresponding *_API_BASE_PATH environment variable.'
);
}
return parsed.hostname || undefined;
} catch (err) {
console.warn(`Invalid API target ${sanitizeForLog(trimmed)}; expected a hostname or URL`);
return undefined;
}
}
// Configurable API target hosts (supports custom endpoints / internal LLM routers)
// Values are normalized to bare hostnames — buildUpstreamPath() prepends https://
containers/api-proxy/server.test.js
Outdated
| it('should preserve hostname with path', () => { | ||
| expect(normalizeApiTarget('https://my-gateway.example.com/some-path')).toBe('my-gateway.example.com/some-path'); |
There was a problem hiding this comment.
This test (and the one above) treats API targets as potentially including a path. In runtime code, the target value is used as a TLS SNI/https.request hostname, so paths are not valid here. Update the tests to reflect that only hostnames are supported (and ideally add a test that values with a path are rejected or normalized to the hostname).
This issue also appears on line 31 of the same file.
| it('should preserve hostname with path', () => { | |
| expect(normalizeApiTarget('https://my-gateway.example.com/some-path')).toBe('my-gateway.example.com/some-path'); | |
| it('should normalize a URL with a path to just the hostname', () => { | |
| expect(normalizeApiTarget('https://my-gateway.example.com/some-path')).toBe('my-gateway.example.com'); |
src/docker-manager.test.ts
Outdated
| it('should preserve hostname with path after scheme strip', () => { | ||
| expect(stripScheme('https://my-gateway.example.com/some-path')).toBe('my-gateway.example.com/some-path'); |
There was a problem hiding this comment.
stripScheme() preserves /some-path after stripping the scheme, but API targets are documented/used as hostnames (see WrapperConfig docs) and are passed as hostname/SNI in the sidecar. Allowing paths here will lead to invalid hostnames at runtime; consider updating this test (and/or stripScheme) to normalize to hostname only, or explicitly reject values containing /.
| it('should preserve hostname with path after scheme strip', () => { | |
| expect(stripScheme('https://my-gateway.example.com/some-path')).toBe('my-gateway.example.com/some-path'); | |
| it('should normalize URL with path to hostname only', () => { | |
| expect(stripScheme('https://my-gateway.example.com/some-path')).toBe('my-gateway.example.com'); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@copilot apply changes based on the comments in this thread |
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.
Use URL parsing in both normalizeApiTarget() and stripScheme() to extract only the hostname, discarding path, query, fragment, port, and credentials. Paths must use the separate *_API_BASE_PATH settings. Addresses review feedback that target values are used as TLS SNI / hostname in https.request(), so including paths would break requests. Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/ab65b061-c924-41d8-a573-dd954497def2
Applied in commits
|
Smoke Test Results✅ GitHub MCP — Last 2 merged PRs: "fix: add retry logic to apt-get upgrade in agent Dockerfile" (#1781), "fix: share mcpg network namespace to fix TLS hostname verification" (#1778) Overall: PASS
|
🔥 Smoke Test Results
Overall: PASS PR by @lpcox · reviewer:
|
|
Smoke test results:
|
Chroot Version Comparison Results
Overall: ❌ FAILED — Python and Node.js versions differ between host and chroot environment.
|
Smoke Test: GitHub Actions Services Connectivity ✅All checks passed:
Note:
|
When
--*-api-targetvalues include anhttps://prefix (common via GitHub Actions expressions like${{ vars.ANTHROPIC_BASE_URL }}), the API proxy constructs malformed URLs likehttps://https://hostthat Squid rejects with 403. The gh-aw compiler'sextractAPITargetHost()strips schemes at compile time, but${{ ... }}expressions are opaque until runtime.Changes
containers/api-proxy/server.js—normalizeApiTarget()now usesnew URL()to extract the bare hostname, discarding scheme, path, port, query, fragment, and credentials. Warns when unsupported components are present (path config belongs in*_API_BASE_PATH).src/docker-manager.ts—stripScheme()similarly uses URL parsing to return hostname only, as a belt-and-suspenders layer before values reach the container.Why URL parsing, not regex
targetHostis passed directly tohttps.request({ hostname })andtls.connect({ servername }). A value likemy-gateway.example.com/some-pathis an invalid hostname/SNI and would break TLS negotiation. Simples/^https?:\/\///was insufficient.