fix: add bare API proxy targets as https:// only to prevent over-broad allowlisting#5130
Conversation
Bare custom API proxy targets (e.g. custom.openai.com) were being added to the allowed domains list twice: once as the raw hostname and once as https://custom.openai.com. Because bare entries are treated as both HTTP and HTTPS by the Squid ACL, this was more permissive than intended. This change updates resolveApiTargetsToAllowedDomains so that: - Bare hostnames are promoted to https:// only - Explicit http:// targets are preserved as-is (intentional HTTP) - Explicit https:// targets are preserved as-is (no double-prefix) Tests and the integration test debug-message assertion are updated to match the new behaviour.
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
This PR tightens Squid allowlisting for custom API proxy targets by ensuring bare hostnames are only allowlisted as https://<host> (rather than also allowlisting the bare hostname, which implicitly permits both HTTP and HTTPS).
Changes:
- Update
resolveApiTargetsToAllowedDomains()to normalize API targets in a single pass and promote bare hostnames tohttps://only. - Adjust unit tests to assert bare hostnames are not inserted into
allowedDomains. - Update an integration test assertion to match the new debug log message.
Show a summary per file
| File | Description |
|---|---|
| src/api-proxy-config.ts | Changes API target → allowedDomains normalization to avoid over-broad allowlisting and updates debug logging. |
| src/api-proxy-config-domains.test.ts | Updates assertions so bare hostnames are no longer expected in allowedDomains. |
| tests/integration/api-target-allowlist.test.ts | Updates debug-log string expectation for auto-added API targets. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
| for (const target of normalizedApiTargets) { | ||
|
|
||
| // Ensure auto-added API targets are explicitly HTTPS to avoid over-broad HTTP+HTTPS allowlisting | ||
| const normalizedTarget = /^https?:\/\//.test(target) ? target : `https://${target}`; | ||
|
|
||
| if (!allowedDomains.includes(normalizedTarget)) { | ||
| allowedDomains.push(normalizedTarget); | ||
| debug(`Automatically added API target to allowlist: ${normalizedTarget}`); | ||
| debug(`Auto-added API target to allowed domains: ${normalizedTarget}`); |
| for (const target of normalizedApiTargets) { | ||
|
|
||
| // Ensure auto-added API targets are explicitly HTTPS to avoid over-broad HTTP+HTTPS allowlisting | ||
| const normalizedTarget = /^https?:\/\//.test(target) ? target : `https://${target}`; | ||
|
|
||
| if (!allowedDomains.includes(normalizedTarget)) { |
|
@copilot address review feedback and make sure linting tests pass |
|
⏳ Copilot review left inline comments. @copilot To proceed:
|
Both review findings addressed in commit 1c13fbd:
Tests covering both cases were added; all 95 unit tests pass and lint is clean. |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓 |
|
❌ Contribution Check failed. Please review the logs for details. |
|
🔌 Smoke Services — All services reachable! ✅ |
|
✅ Smoke Gemini completed. All facets verified. 💎 Testing safeoutputs |
|
✅ Build Test Suite completed successfully! |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
❌ Smoke Claude failed |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
🚀 Security Guard has started processing this pull request |
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓 |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
🔬 Smoke Test: PAT Auth — PASS
Auth mode: PAT (COPILOT_GITHUB_TOKEN)
|
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
🔬 Smoke Test ResultsPR: fix: add bare API proxy targets as https:// only to prevent over-broad allowlisting
Overall: FAIL — pre-step template variables were not resolved (
|
|
fix: add bare API proxy targets as https:// only to prevent over-broad allowlisting ✅ MCP connectivity PASS — Running in direct BYOK mode (COPILOT_PROVIDER_API_KEY + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw)
|
This comment has been minimized.
This comment has been minimized.
|
Merged PRs: docs: sync schemas and specs with source changes; [Test Coverage] Improve test coverage for network-options, environment-builder, log-parser, workspace-mounts
|
Smoke Test Results for Gemini
Overall Status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
🔍 Smoke Test: API Proxy OTEL Tracing — Results
All scenarios pass. 🎉
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Smoke Test Results — FAIL
|
Smoke Test: Copilot BYOK (Direct Mode) ✅ PASS
Mode: Direct BYOK (COPILOT_PROVIDER_API_KEY) → api-proxy sidecar → api.githubcopilot.com All systems operational.
|
Chroot Runtime Version Comparison
Result: Not all versions match —
|
|
Running in direct BYOK mode (AWF_AUTH_TYPE=github-oidc + AWF_AUTH_AZURE_* + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw) authenticated via Microsoft Entra Overall: PASS
|
Bare custom API proxy targets (e.g.
custom.openai.com) were being added to the Squid allowlist as both the raw hostname andhttps://custom.openai.com. Because bare entries match both HTTP and HTTPS in the Squid ACL, this was broader than intended.Changes
src/api-proxy-config.ts— Collapse the two-pass logic inresolveApiTargetsToAllowedDomainsinto a single pass:https://<host>onlyhttp://targets → preserved as-is (intentional HTTP deployments)https://targets → preserved as-is (no double-prefix)src/api-proxy-config-domains.test.ts— Update assertions to verify bare hostnames are not inserted and only thehttps://form appears.tests/integration/api-target-allowlist.test.ts— Update debug-message string to match the new log format.Before/after behaviour:
Explicit
http://custom.openai.cominputs are preserved as-is for deployments that intentionally target an HTTP endpoint.