Compiler: auto-add GHES domains to --allow-domains when engine.api-target is set#21301
Merged
Compiler: auto-add GHES domains to --allow-domains when engine.api-target is set#21301
--allow-domains when engine.api-target is set#21301Conversation
…rget is set When engine.api-target is set (e.g., "api.acme.ghe.com" for a GHES instance), both the API domain and the base hostname are now automatically added to: - --allow-domains (AWF firewall flag) - GH_AW_ALLOWED_DOMAINS (environment variable for sanitization) This prevents the need for manual lock file edits after every recompile." Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix compiler to auto-add GHES domains to --allow-domains
Compiler: auto-add GHES domains to Mar 16, 2026
--allow-domains when engine.api-target is set
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes GHES/GHEC engine.api-target workflows getting blocked after recompile by automatically adding the relevant API target hostnames into the runtime AWF --allow-domains list and the compiler-side GH_AW_ALLOWED_DOMAINS sanitization allowlist.
Changes:
- Added
GetAPITargetDomains()andmergeAPITargetDomains()to derive and merge api-target-related domains into allowlists. - Updated
computeAllowedDomainsForSanitization()to include api-target domains so sanitization stays in sync with firewall configuration. - Updated Copilot/Claude/Codex/Gemini engines to merge api-target domains into the AWF
--allow-domainsvalue used at runtime.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/domains.go | Adds api-target domain derivation/merge helpers; includes api-target domains in compiler-side allowed-domain computation for sanitization. |
| pkg/workflow/domains_test.go | Adds unit tests for GetAPITargetDomains() and mergeAPITargetDomains(). |
| pkg/workflow/copilot_engine_execution.go | Merges api-target domains into Copilot’s runtime --allow-domains. |
| pkg/workflow/claude_engine.go | Merges api-target domains into Claude’s runtime --allow-domains. |
| pkg/workflow/codex_engine.go | Merges api-target domains into Codex’s runtime --allow-domains. |
| pkg/workflow/gemini_engine.go | Merges api-target domains into Gemini’s runtime --allow-domains. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+727
to
+732
| // Add GHES/custom API target domains so GH_AW_ALLOWED_DOMAINS stays in sync with --allow-domains | ||
| if data.EngineConfig != nil && data.EngineConfig.APITarget != "" { | ||
| base = mergeAPITargetDomains(base, data.EngineConfig.APITarget) | ||
| } | ||
|
|
||
| return base |
Contributor
Author
There was a problem hiding this comment.
Added regression tests in commit 0872ff5:
- Extended
TestComputeAllowedDomainsForSanitizationwith two new table-driven cases: one assertingapi.acme.ghe.com+acme.ghe.comappear in the sanitization domain set, and one asserting the base hostname is not added for non-api.prefixed targets. - Added
TestAPITargetDomainsInCompiledWorkflow(integration,//go:build integration) that compiles a full workflow withengine.api-targetconfigured and verifies both--allow-domainsin the AWF command andGH_AW_ALLOWED_DOMAINSin the env block contain the expected domains. Uses exact CSV-membership checks (viaextractQuotedCSVhelper) for the "not present" assertions to avoid substring false-positives.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- Extend TestComputeAllowedDomainsForSanitization with api-target test cases (GHES and non-api. prefix hostname), with exact-membership checks for the "not present" assertions to avoid false positives from substring matching. - Add integration test TestAPITargetDomainsInCompiledWorkflow that compiles a full workflow with engine.api-target and asserts both --allow-domains and GH_AW_ALLOWED_DOMAINS in the lock file contain the expected domains. - Add extractQuotedCSV helper for exact domain membership checking." Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When
engine.api-targetis set for a GHES instance, the compiler correctly passed--copilot-api-targetto AWF but never added the GHES hostnames to the firewall allow-lists — meaning every recompile silently broke GHES workflows by blocking API traffic.Changes
domains.go: AddedGetAPITargetDomains(apiTarget)which derives the API domain and, only when the target starts withapi., the base hostname (e.g.api.acme.ghe.com→["api.acme.ghe.com", "acme.ghe.com"];copilot.corp.example.com→["copilot.corp.example.com"]). This scoping prevents unintended egress broadening for non-api.targets. AddedmergeAPITargetDomainshelper to merge these into an existing domain string. UpdatedcomputeAllowedDomainsForSanitizationto include api-target domains inGH_AW_ALLOWED_DOMAINS.Engine files (
copilot_engine_execution.go,claude_engine.go,codex_engine.go,gemini_engine.go): Each now merges api-target domains intoallowedDomainsafter the base domain computation, keeping--allow-domainsandGH_AW_ALLOWED_DOMAINSin sync.Tests (
allowed_domains_sanitization_test.go,domains_test.go): ExtendedTestComputeAllowedDomainsForSanitizationwith api-target cases. Added integration testTestAPITargetDomainsInCompiledWorkflowthat compiles a full workflow withengine.api-targetand asserts both--allow-domainsandGH_AW_ALLOWED_DOMAINSin the lock file contain the expected domains.Example
Now automatically produces in the compiled lock file:
No manual lock file edits required, and the domains survive recompile.
📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.