-
Notifications
You must be signed in to change notification settings - Fork 17
fix(proxy): derive api.subdomain.ghe.com for GHEC domains #1072
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
243a6a5
Initial plan
Claude 085cf29
fix(api-proxy): derive api.subdomain.ghe.com for ghec domains
Claude 43f446f
refactor(api-proxy): export function for tests and improve docs
Claude b14291f
fix: add explicit execute directive to smoke-codex to prevent noop (#…
Copilot 14088d7
Merge branch 'main' into claude/fix-api-proxy-config
Mossaka 5a3d113
Merge remote-tracking branch 'origin/main' into claude/fix-api-proxy-…
Mossaka File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| /** | ||
| * Tests for API Proxy Server functions | ||
| */ | ||
|
|
||
| const { deriveCopilotApiTarget } = require('./server'); | ||
|
|
||
| describe('deriveCopilotApiTarget', () => { | ||
| let originalEnv; | ||
|
|
||
| beforeEach(() => { | ||
| // Save original environment | ||
| originalEnv = { ...process.env }; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| // Restore original environment | ||
| process.env = originalEnv; | ||
| }); | ||
|
|
||
| test('should return default api.githubcopilot.com when no env vars set', () => { | ||
| delete process.env.COPILOT_API_TARGET; | ||
| delete process.env.GITHUB_SERVER_URL; | ||
|
|
||
| const target = deriveCopilotApiTarget(); | ||
| expect(target).toBe('api.githubcopilot.com'); | ||
| }); | ||
|
|
||
| test('should use COPILOT_API_TARGET when explicitly set', () => { | ||
| process.env.COPILOT_API_TARGET = 'custom.api.example.com'; | ||
| const target = deriveCopilotApiTarget(); | ||
| expect(target).toBe('custom.api.example.com'); | ||
| }); | ||
|
|
||
| test('should prioritize COPILOT_API_TARGET over GITHUB_SERVER_URL', () => { | ||
| process.env.COPILOT_API_TARGET = 'custom.api.example.com'; | ||
| process.env.GITHUB_SERVER_URL = 'https://mycompany.ghe.com'; | ||
| const target = deriveCopilotApiTarget(); | ||
| expect(target).toBe('custom.api.example.com'); | ||
| }); | ||
|
|
||
| test('should return api.githubcopilot.com for github.com', () => { | ||
| process.env.GITHUB_SERVER_URL = 'https://github.com'; | ||
| const target = deriveCopilotApiTarget(); | ||
| expect(target).toBe('api.githubcopilot.com'); | ||
| }); | ||
|
|
||
| test('should derive api.SUBDOMAIN.ghe.com for *.ghe.com domains', () => { | ||
| process.env.GITHUB_SERVER_URL = 'https://mycompany.ghe.com'; | ||
| const target = deriveCopilotApiTarget(); | ||
| expect(target).toBe('api.mycompany.ghe.com'); | ||
| }); | ||
|
|
||
| test('should derive api.SUBDOMAIN.ghe.com for different *.ghe.com subdomain', () => { | ||
| process.env.GITHUB_SERVER_URL = 'https://acme-corp.ghe.com'; | ||
| const target = deriveCopilotApiTarget(); | ||
| expect(target).toBe('api.acme-corp.ghe.com'); | ||
| }); | ||
|
|
||
| test('should use api.enterprise.githubcopilot.com for GHES (non-.ghe.com enterprise)', () => { | ||
| process.env.GITHUB_SERVER_URL = 'https://github.enterprise.com'; | ||
| const target = deriveCopilotApiTarget(); | ||
| expect(target).toBe('api.enterprise.githubcopilot.com'); | ||
| }); | ||
|
|
||
| test('should use api.enterprise.githubcopilot.com for custom GHES domain', () => { | ||
| process.env.GITHUB_SERVER_URL = 'https://git.mycompany.com'; | ||
| const target = deriveCopilotApiTarget(); | ||
| expect(target).toBe('api.enterprise.githubcopilot.com'); | ||
| }); | ||
|
|
||
| test('should handle GITHUB_SERVER_URL without protocol gracefully', () => { | ||
| process.env.GITHUB_SERVER_URL = 'mycompany.ghe.com'; | ||
| const target = deriveCopilotApiTarget(); | ||
| // Invalid URL, should fall back to default | ||
| expect(target).toBe('api.githubcopilot.com'); | ||
| }); | ||
|
|
||
| test('should handle invalid GITHUB_SERVER_URL gracefully', () => { | ||
| process.env.GITHUB_SERVER_URL = 'not-a-valid-url'; | ||
| const target = deriveCopilotApiTarget(); | ||
| expect(target).toBe('api.githubcopilot.com'); | ||
| }); | ||
|
|
||
| test('should handle GITHUB_SERVER_URL with port', () => { | ||
| process.env.GITHUB_SERVER_URL = 'https://mycompany.ghe.com:443'; | ||
| const target = deriveCopilotApiTarget(); | ||
| expect(target).toBe('api.mycompany.ghe.com'); | ||
| }); | ||
|
|
||
| test('should handle GITHUB_SERVER_URL with path', () => { | ||
| process.env.GITHUB_SERVER_URL = 'https://mycompany.ghe.com/some/path'; | ||
| const target = deriveCopilotApiTarget(); | ||
| expect(target).toBe('api.mycompany.ghe.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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs example allowlist includes mycompany.ghe.com, but the api-proxy only makes outbound Copilot requests to the derived API host (e.g., api.mycompany.ghe.com). Listing extra domains encourages a broader-than-necessary egress policy. Consider narrowing the example to only the required Copilot API domain (and mention adding additional domains only if the user’s workflow needs them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated examples to use wildcard pattern
*.mycompany.ghe.comwhich matches all subdomains includingapi.mycompany.ghe.com. Added explanation that additional domains should only be added if needed for the user's workflow. (commit 43f446f)