From e500ccc40eccdc13dc01743a7d125805fd982a82 Mon Sep 17 00:00:00 2001 From: OrcaWin <293788423+OrcaWin@users.noreply.github.com> Date: Mon, 20 Jul 2026 20:57:18 -0700 Subject: [PATCH] fix(github): preserve GHES non-default auth ports --- src/main/git/runner-gh-host-args.test.ts | 4 +- .../github/client-pr-local-runtime.test.ts | 14 +-- src/main/github/github-api-repository.test.ts | 6 +- .../github-enterprise-repository.test.ts | 110 +++++++++++++++--- .../github/github-enterprise-repository.ts | 15 ++- 5 files changed, 114 insertions(+), 35 deletions(-) diff --git a/src/main/git/runner-gh-host-args.test.ts b/src/main/git/runner-gh-host-args.test.ts index 0bee44eac74..708f2d732aa 100644 --- a/src/main/git/runner-gh-host-args.test.ts +++ b/src/main/git/runner-gh-host-args.test.ts @@ -21,10 +21,10 @@ describe('applyGhHostToArgs', () => { 'github.com', 'rate_limit' ]) - expect(applyGhHostToArgs(['api', 'repos/a/b/pulls'], 'github.acme-corp.com')).toEqual([ + expect(applyGhHostToArgs(['api', 'repos/a/b/pulls'], 'github.acme-corp.com:8443')).toEqual([ 'api', '--hostname', - 'github.acme-corp.com', + 'github.acme-corp.com:8443', 'repos/a/b/pulls' ]) }) diff --git a/src/main/github/client-pr-local-runtime.test.ts b/src/main/github/client-pr-local-runtime.test.ts index 07a69a6a4ee..a2aa0595c74 100644 --- a/src/main/github/client-pr-local-runtime.test.ts +++ b/src/main/github/client-pr-local-runtime.test.ts @@ -363,11 +363,11 @@ describe('GitHub PR local runtime routing', () => { expect(ghExecFileAsyncMock).not.toHaveBeenCalled() }) - it('host-qualifies SSH-backed GitHub Enterprise review reads and mutations', async () => { + it('preserves a ported GHES host in SSH-backed review reads and mutations', async () => { const enterpriseRepo = { owner: 'team', repo: 'orca', - host: 'github.acme-corp.com' + host: 'github.acme-corp.com:8443' } getOwnerRepoMock.mockResolvedValue(null) getEnterpriseGitHubRepoSlugMock.mockResolvedValue(enterpriseRepo) @@ -392,7 +392,7 @@ describe('GitHub PR local runtime routing', () => { number: 7, title: 'Enterprise PR', state: 'OPEN', - url: 'https://github.acme-corp.com/team/orca/pull/7', + url: 'https://github.acme-corp.com:8443/team/orca/pull/7', labels: [], updatedAt: '2026-07-16T00:00:00Z', author: { login: 'pr-author' }, @@ -430,7 +430,7 @@ describe('GitHub PR local runtime routing', () => { name: 'lint', status: 'completed', conclusion: 'failure', - details_url: 'https://github.acme-corp.com/team/orca/actions/runs/77/job/88' + details_url: 'https://github.acme-corp.com:8443/team/orca/actions/runs/77/job/88' } ] }) @@ -449,7 +449,7 @@ describe('GitHub PR local runtime routing', () => { name: 'lint', status: 'completed', conclusion: 'failure', - details_url: 'https://github.acme-corp.com/team/orca/actions/runs/77/job/88', + details_url: 'https://github.acme-corp.com:8443/team/orca/actions/runs/77/job/88', output: { title: 'Lint failed', summary: 'One error' } }) } @@ -557,7 +557,7 @@ describe('GitHub PR local runtime routing', () => { // The runner host-qualifies argv at spawn time from options.host, so the // mocked call sees the unqualified --repo plus the host in exec options. expect(prViewCall?.[0]).toEqual(expect.arrayContaining(['--repo', 'team/orca'])) - expect(prViewCall?.[1]).toEqual({ host: 'github.acme-corp.com' }) + expect(prViewCall?.[1]).toEqual({ host: 'github.acme-corp.com:8443' }) const prCalls = ghExecFileAsyncMock.mock.calls.filter(([args]) => args[0] === 'pr') expect( prCalls.every( @@ -570,7 +570,7 @@ describe('GitHub PR local runtime routing', () => { apiCalls.every( ([args, options]) => !args.includes('--hostname') && - options.host === 'github.acme-corp.com' && + options.host === 'github.acme-corp.com:8443' && options.cwd === undefined && options.wslDistro === undefined ) diff --git a/src/main/github/github-api-repository.test.ts b/src/main/github/github-api-repository.test.ts index fdf921a92ad..2ddfc507630 100644 --- a/src/main/github/github-api-repository.test.ts +++ b/src/main/github/github-api-repository.test.ts @@ -61,7 +61,7 @@ describe('githubHostExecOptions', () => { describe('resolveGitHubRepoExecution', () => { it('combines local repository and GitHub host execution options', async () => { - const ownerRepo = { owner: 'acme', repo: 'widgets', host: 'github.acme-corp.com' } + const ownerRepo = { owner: 'acme', repo: 'widgets', host: 'github.acme-corp.com:8443' } isGitHubHostAuthenticatedMock.mockResolvedValue(true) await expect( @@ -71,11 +71,11 @@ describe('resolveGitHubRepoExecution', () => { ghOptions: { cwd: '/repo', wslDistro: 'Ubuntu', - host: 'github.acme-corp.com' + host: 'github.acme-corp.com:8443' } }) expect(isGitHubHostAuthenticatedMock).toHaveBeenCalledWith( - 'github.acme-corp.com', + 'github.acme-corp.com:8443', '/repo', null, { wslDistro: 'Ubuntu' } diff --git a/src/main/github/github-enterprise-repository.test.ts b/src/main/github/github-enterprise-repository.test.ts index 0a361ea1323..b2c453582e3 100644 --- a/src/main/github/github-enterprise-repository.test.ts +++ b/src/main/github/github-enterprise-repository.test.ts @@ -28,10 +28,16 @@ function mockOriginRemote(url: string): void { }) } -// gh exit 0 for `auth status --hostname ` means logged in to that host. +// gh auth status inventory entries represent hosts with configured credentials. function mockHostAuthenticated(host = 'github.acme-corp.com'): void { + mockAuthenticatedHosts([host]) +} + +function mockAuthenticatedHosts(hosts: string[]): void { ghExecFileAsyncMock.mockResolvedValue({ - stdout: `${host}\n ✓ Logged in to ${host} account kelora (keyring)`, + stdout: hosts + .map((host) => `${host}\n ✓ Logged in to ${host} account kelora (keyring)`) + .join('\n'), stderr: '' }) } @@ -89,14 +95,67 @@ describe('getEnterpriseGitHubRepoSlug', () => { }) }) - it('uses the auth inventory host when an HTTPS remote includes a non-default port', async () => { - mockOriginRemote('https://ghe.acme.com:8443/team/orca.git') + it.each([ + [['ghe.acme.com', 'ghe.acme.com:9443', 'ghe.acme.com:8443']], + [['ghe.acme.com:8443', 'ghe.acme.com:9443', 'ghe.acme.com']] + ])( + 'requires exact host:port authentication regardless of auth inventory order: %j', + async (authenticatedHosts) => { + mockOriginRemote('https://ghe.acme.com:8443/team/orca.git') + mockAuthenticatedHosts(authenticatedHosts) + + await expect(getEnterpriseGitHubRepoSlug('/repo')).resolves.toEqual({ + owner: 'team', + repo: 'orca', + host: 'ghe.acme.com:8443' + }) + } + ) + + it.each([ + 'https://ghe.acme.com:8443/team/orca.git', + 'https://ghe.acme.com:80/team/orca.git', + 'http://ghe.acme.com:443/team/orca.git' + ])('rejects portless authentication for a non-default web endpoint: %s', async (remoteUrl) => { + mockOriginRemote(remoteUrl) mockHostAuthenticated('ghe.acme.com') + await expect(getEnterpriseGitHubRepoSlug('/repo')).resolves.toBeNull() + }) + + it.each(['http://ghe.acme.com:80/team/orca.git', 'https://ghe.acme.com:443/team/orca.git'])( + 'matches a protocol-default port to the portless auth host: %s', + async (remoteUrl) => { + mockOriginRemote(remoteUrl) + mockHostAuthenticated('ghe.acme.com') + + await expect(getEnterpriseGitHubRepoSlug('/repo')).resolves.toEqual({ + owner: 'team', + repo: 'orca', + host: 'ghe.acme.com' + }) + } + ) + + it('preserves exact authentication for an HTTP endpoint on port 443', async () => { + mockOriginRemote('http://ghe.acme.com:443/team/orca.git') + mockHostAuthenticated('ghe.acme.com:443') + await expect(getEnterpriseGitHubRepoSlug('/repo')).resolves.toEqual({ owner: 'team', repo: 'orca', - host: 'ghe.acme.com' + host: 'ghe.acme.com:443' + }) + }) + + it('preserves exact authentication for an HTTPS endpoint on port 80', async () => { + mockOriginRemote('https://ghe.acme.com:80/team/orca.git') + mockHostAuthenticated('ghe.acme.com:80') + + await expect(getEnterpriseGitHubRepoSlug('/repo')).resolves.toEqual({ + owner: 'team', + repo: 'orca', + host: 'ghe.acme.com:80' }) }) @@ -264,17 +323,38 @@ describe('isGitHubHostAuthenticated', () => { expect(ghExecFileAsyncMock.mock.calls.flat()).not.toContain('evil.example.test') }) - it('does not guess between multiple ported auth hosts for one SSH hostname', async () => { - ghExecFileAsyncMock.mockResolvedValue({ - stdout: `ghe.acme.com:8443 - ✓ Logged in to ghe.acme.com:8443 account kelora (keyring) -ghe.acme.com:9443 - ✓ Logged in to ghe.acme.com:9443 account other (keyring)`, - stderr: '' - }) + it.each([ + [['ghe.acme.com:8443', 'ghe.acme.com:9443']], + [['ghe.acme.com:9443', 'ghe.acme.com:8443']] + ])( + 'does not guess between multiple ported auth hosts for one SSH hostname: %j', + async (authenticatedHosts) => { + mockAuthenticatedHosts(authenticatedHosts) - await expect(isGitHubHostAuthenticated('ghe.acme.com', '/repo')).resolves.toBe(false) - }) + await expect(isGitHubHostAuthenticated('ghe.acme.com', '/repo')).resolves.toBe(false) + } + ) + + it.each([ + [ + ['ghe.acme.com', false], + ['ghe.acme.com:8443', true] + ], + [ + ['ghe.acme.com:8443', true], + ['ghe.acme.com', false] + ] + ] as const)( + 'keeps exact-port and ambiguous hostname auth cached independently: %j', + async (...requests) => { + mockAuthenticatedHosts(['ghe.acme.com:8443', 'ghe.acme.com:9443']) + + for (const [host, expected] of requests) { + await expect(isGitHubHostAuthenticated(host, '/repo')).resolves.toBe(expected) + } + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + } + ) it('treats default web ports as the same auth host', async () => { mockHostAuthenticated('ghe.acme.com:443') diff --git a/src/main/github/github-enterprise-repository.ts b/src/main/github/github-enterprise-repository.ts index b6b28e62f4e..f0277068471 100644 --- a/src/main/github/github-enterprise-repository.ts +++ b/src/main/github/github-enterprise-repository.ts @@ -90,10 +90,8 @@ function normalizeGitHubHost(host: string): NormalizedGitHubHost | null { return null } const hostname = match[1] - const rawPort = match[2] ?? null - // Why: default web ports do not distinguish a gh auth host from the same - // hostname without an explicit port. - const port = rawPort === '80' || rawPort === '443' ? null : rawPort + // Why: remote URL parsing already removes protocol-default ports; any port left here identifies the endpoint. + const port = match[2] ?? null return { hostname, port, authority: port ? `${hostname}:${port}` : hostname } } @@ -114,10 +112,11 @@ function authenticatedHostFromInventory(host: string, output: string): string | if (exact) { return exact.authority } - const compatible = inventory.filter( - (candidate) => - candidate.hostname === requested.hostname && (!requested.port || candidate.port === null) - ) + // Why: a non-default web port identifies the API endpoint; portless credentials target a different server. + if (requested.port) { + return null + } + const compatible = inventory.filter((candidate) => candidate.hostname === requested.hostname) // Why: an SSH remote has no API port. Only a unique auth-inventory host can // safely supply it; multiple ported endpoints on one hostname are ambiguous. return compatible.length === 1 ? compatible[0].authority : null