From 8e9b5c908c8dc61d27b292e3c28550200011cbd4 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 17 Aug 2026 00:12:14 -0700 Subject: [PATCH] fix(github): fail closed instead of running client git against a remote repoPath when the SSH provider is unregistered (#14945) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(github): fail closed when the SSH git provider is gone getCurrentHeadOid and probeTrackedUpstreamBranches only routed through the SSH provider when one was registered. With connectionId set but the provider unregistered (dropped connection, not yet reattached) they fell through to client-side git with cwd pointing at the remote repoPath — on a machine with a same-named local path that silently answers for the wrong repository. getCurrentHeadOid feeds shouldHideMergedImplicitPR, so a wrong OID changes which PR the UI attributes to a worktree. Both now take their existing unknown path (null / probeFailed) instead, matching repo-default-branch.ts. Local and WSL routing is unchanged. * fix(github): preserve PR state when SSH probes fail * fix(github): keep failed SSH discovery unverifiable * fix(github): propagate SSH identity failures * fix(github): scope verified SSH identity probes * test(github): preserve tolerant resolver calls * fix(github): preserve indeterminate auth discovery * fix(github): isolate SSH repository probe generations * test(github): expose SSH probe generation in mocks --- src/main/git/remote-url-probe.test.ts | 9 + src/main/git/remote-url-probe.ts | 13 +- .../github/client-pr-local-runtime.test.ts | 1 + ...nt-ssh-provider-execution-boundary.test.ts | 266 ++++++++++++++++++ src/main/github/client-test-mocks.ts | 12 +- src/main/github/client.ts | 46 ++- src/main/github/gh-utils.ts | 1 + .../github/github-api-repository-probe.ts | 25 ++ .../github-api-repository-validation.ts | 20 ++ src/main/github/github-api-repository.test.ts | 66 +++++ src/main/github/github-api-repository.ts | 115 ++++---- .../github/github-enterprise-repository.ts | 33 ++- src/main/github/github-repository-host.ts | 18 ++ ...repository-identity.ssh-host-alias.test.ts | 67 ++++- src/main/github/github-repository-identity.ts | 47 +++- 15 files changed, 641 insertions(+), 98 deletions(-) create mode 100644 src/main/github/client-ssh-provider-execution-boundary.test.ts create mode 100644 src/main/github/github-api-repository-probe.ts create mode 100644 src/main/github/github-api-repository-validation.ts create mode 100644 src/main/github/github-repository-host.ts diff --git a/src/main/git/remote-url-probe.test.ts b/src/main/git/remote-url-probe.test.ts index 8a7672cb229..f9e0348c3ac 100644 --- a/src/main/git/remote-url-probe.test.ts +++ b/src/main/git/remote-url-probe.test.ts @@ -97,6 +97,15 @@ describe('remote URL probe', () => { await expect(assertRemoteUrlReadable({ repoPath: '/repo' })).resolves.toBeUndefined() }) + it('keeps a failed SSH command unverifiable even without a transport error string', async () => { + const failure = new Error('relay request failed') + getSshGitProviderMock.mockReturnValue({ exec: vi.fn().mockRejectedValue(failure) }) + + await expect( + assertRemoteUrlReadable({ repoPath: '/repo', connectionId: 'ssh-1' }) + ).rejects.toBe(failure) + }) + it('does not turn a missing SSH provider into a false readable result', async () => { getSshGitProviderMock.mockReturnValue(null) diff --git a/src/main/git/remote-url-probe.ts b/src/main/git/remote-url-probe.ts index 7b18c1deac9..d237bdd98fa 100644 --- a/src/main/git/remote-url-probe.ts +++ b/src/main/git/remote-url-probe.ts @@ -3,6 +3,7 @@ import { SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE } from '../providers/ssh-git-dispatch' import { gitExecFileAsync } from './runner' +import { isStableMissingGitRemoteError } from './stable-missing-git-remote-error' /** * The `git remote get-url` probe every forge integration runs to decide whether @@ -91,10 +92,9 @@ export function isTransientGitProbeError(error: unknown): boolean { } /** - * Throws when the remote could not be read because the probe was wedged or its - * transport was gone, and returns normally for every answer — including a repo - * with no remote at all. Lets a caller that treats "nothing found" as a cacheable - * answer tell that apart from a lookup that never got to ask. + * Throws when the remote could not be read, and returns normally for every + * answer — including a repo with no remote at all. Lets a caller that treats + * "nothing found" as cacheable tell that apart from a lookup that never got to ask. */ export async function assertRemoteUrlReadable( context: RemoteUrlProbeContext, @@ -106,8 +106,9 @@ export async function assertRemoteUrlReadable( try { await readRemoteUrl(context, remoteName) } catch (error) { - if (isTransientGitProbeError(error)) { - throw error + if (isStableMissingGitRemoteError(error)) { + return } + throw error } } diff --git a/src/main/github/client-pr-local-runtime.test.ts b/src/main/github/client-pr-local-runtime.test.ts index 543d6f239f2..0633b137b1c 100644 --- a/src/main/github/client-pr-local-runtime.test.ts +++ b/src/main/github/client-pr-local-runtime.test.ts @@ -85,6 +85,7 @@ vi.mock('./github-enterprise-repository', () => ({ })) vi.mock('../providers/ssh-git-dispatch', () => ({ + getSshGitProviderGeneration: vi.fn(() => 0), getSshGitProvider: vi.fn() })) diff --git a/src/main/github/client-ssh-provider-execution-boundary.test.ts b/src/main/github/client-ssh-provider-execution-boundary.test.ts new file mode 100644 index 00000000000..1f9bb1b1704 --- /dev/null +++ b/src/main/github/client-ssh-provider-execution-boundary.test.ts @@ -0,0 +1,266 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GithubApiRepositoryModule from './github-api-repository' +import type * as GitHubEnterpriseRepositoryModule from './github-enterprise-repository' + +const { clientMocks, moduleMocks } = await vi.hoisted(async () => { + const moduleMocks = await import('./client-test-mocks') + return { clientMocks: moduleMocks.createGitHubClientMocks(), moduleMocks } +}) + +vi.mock('./gh-utils', () => moduleMocks.ghUtilsModuleMock(clientMocks)) +vi.mock('../git/runner', () => moduleMocks.gitRunnerModuleMock(clientMocks)) +vi.mock('../providers/ssh-git-dispatch', () => moduleMocks.sshGitDispatchModuleMock(clientMocks)) +vi.mock('./local-git-config-signature', () => + moduleMocks.localGitConfigSignatureModuleMock(clientMocks) +) +vi.mock('./github-enterprise-repository', async (importOriginal) => + moduleMocks.githubEnterpriseRepositoryModuleMock( + await importOriginal() + ) +) +vi.mock('./rate-limit', () => moduleMocks.rateLimitModuleMock(clientMocks)) +vi.mock('./github-api-repository', async (importOriginal) => + moduleMocks.githubApiRepositoryModuleMock( + clientMocks, + await importOriginal() + ) +) + +import { getPRForBranch, getPRForBranchOutcome } from './client' +import { resetPRForBranchMocks } from './client-test-harness' + +const { + ghExecFileAsyncMock, + getOwnerRepoMock, + gitExecFileAsyncMock, + getSshGitProviderMock, + resolvePRRepositoryCandidatesMock +} = clientMocks + +const MERGED_BRANCH = 'fix-tab-strip-layout-test' +const MERGED_HEAD_OID = 'current-head-oid' + +function mockMergedBranchPRLookup(): void { + ghExecFileAsyncMock + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + number: 5875, + title: 'Merged current branch PR', + state: 'closed', + merged_at: '2026-06-20T04:53:05Z', + html_url: 'https://github.com/acme/widgets/pull/5875', + updated_at: '2026-06-20T04:53:05Z', + draft: false, + mergeable_state: 'clean', + head: { ref: MERGED_BRANCH, sha: MERGED_HEAD_OID }, + base: { ref: 'main', sha: 'base-oid' } + } + ]) + }) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 5875, + title: 'Merged current branch PR', + state: 'MERGED', + url: 'https://github.com/acme/widgets/pull/5875', + statusCheckRollup: [], + updatedAt: '2026-06-20T04:53:05Z', + isDraft: false, + mergeable: 'MERGEABLE', + baseRefName: 'main', + headRefName: MERGED_BRANCH, + baseRefOid: 'base-oid', + headRefOid: MERGED_HEAD_OID + }) + }) +} + +function mockUpstreamOnlyPRLookup(): void { + ghExecFileAsyncMock + .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + number: 78, + title: 'Upstream branch PR', + state: 'open', + html_url: 'https://github.com/acme/widgets/pull/78', + updated_at: '2026-03-28T00:00:00Z', + draft: false, + mergeable: true, + base: { ref: 'main', sha: 'base-oid' }, + head: { ref: 'contributor/original', sha: 'upstream-head-oid' } + } + ]) + }) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 78, + title: 'Hydrated upstream branch PR', + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/78', + statusCheckRollup: [], + updatedAt: '2026-03-28T00:00:00Z', + isDraft: false, + mergeable: 'MERGEABLE', + baseRefName: 'main', + headRefName: 'contributor/original', + baseRefOid: 'base-oid', + headRefOid: 'upstream-head-oid' + }) + }) +} + +// The strict execution boundary: for an SSH-hosted repoPath, an unregistered +// provider means "we could not ask" — it must never degrade into a client-side +// git run, which on a same-named local path answers for the wrong repository. +describe('getPRForBranch SSH execution boundary', () => { + beforeEach(() => { + resetPRForBranchMocks(clientMocks) + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + }) + + it('does not rev-parse HEAD locally when the SSH provider is unregistered', async () => { + getSshGitProviderMock.mockReturnValue(undefined) + mockMergedBranchPRLookup() + gitExecFileAsyncMock.mockResolvedValue({ stdout: `${MERGED_HEAD_OID}\n`, stderr: '' }) + + const outcome = await getPRForBranchOutcome('/remote/repo', MERGED_BRANCH, null, 'ssh-1') + + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + expect(outcome).toMatchObject({ kind: 'upstream-error', errorType: 'unknown' }) + }) + + it('reports an upstream error when SSH candidate discovery is unverifiable', async () => { + resolvePRRepositoryCandidatesMock.mockRejectedValue( + new Error('Remote repository identity is unverifiable.') + ) + + const outcome = await getPRForBranchOutcome('/remote/repo', MERGED_BRANCH, null, 'ssh-1') + + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + expect(outcome).toMatchObject({ kind: 'upstream-error', errorType: 'unknown' }) + }) + + it('keeps a verified empty SSH candidate set as no PR', async () => { + resolvePRRepositoryCandidatesMock.mockResolvedValue({ candidates: [], headRepo: null }) + + const outcome = await getPRForBranchOutcome('/remote/repo', MERGED_BRANCH, null, 'ssh-1') + + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + expect(outcome).toMatchObject({ kind: 'no-pr' }) + }) + + it('rev-parses HEAD through the SSH provider when it is registered', async () => { + const sshGitProvider = { + exec: vi.fn(async (args: string[]) => + args[0] === 'rev-parse' && args[1] === 'HEAD' + ? { stdout: `${MERGED_HEAD_OID}\n`, stderr: '' } + : { stdout: '', stderr: '' } + ) + } + getSshGitProviderMock.mockReturnValue(sshGitProvider) + mockMergedBranchPRLookup() + + const pr = await getPRForBranch('/remote/repo', MERGED_BRANCH, null, 'ssh-1') + + expect(sshGitProvider.exec).toHaveBeenCalledWith(['rev-parse', 'HEAD'], '/remote/repo') + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + expect(pr).toMatchObject({ number: 5875, state: 'merged', headSha: MERGED_HEAD_OID }) + }) + + it('rev-parses HEAD through the local runtime for a WSL repository', async () => { + mockMergedBranchPRLookup() + gitExecFileAsyncMock.mockResolvedValue({ stdout: `${MERGED_HEAD_OID}\n`, stderr: '' }) + + const pr = await getPRForBranch('/repo-root', MERGED_BRANCH, null, null, null, { + localGitExecOptions: { wslDistro: 'Ubuntu' } + }) + + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['rev-parse', 'HEAD'], { + cwd: '/repo-root', + wslDistro: 'Ubuntu' + }) + expect(pr).toMatchObject({ number: 5875, state: 'merged', headSha: MERGED_HEAD_OID }) + }) + + it('does not read tracked upstreams locally when the SSH provider is unregistered', async () => { + getSshGitProviderMock.mockReturnValue(undefined) + mockUpstreamOnlyPRLookup() + gitExecFileAsyncMock.mockResolvedValue({ + stdout: `${MERGED_BRANCH}\0origin/contributor/original\n`, + stderr: '' + }) + + const outcome = await getPRForBranchOutcome('/remote/repo', MERGED_BRANCH, null, 'ssh-1') + + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + expect(outcome).toMatchObject({ kind: 'upstream-error', errorType: 'unknown' }) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) + + it('reports an upstream error when the SSH HEAD probe loses its provider mid-flight', async () => { + getSshGitProviderMock.mockReturnValue({ + exec: vi.fn().mockRejectedValue(new Error('SSH transport closed')) + }) + mockMergedBranchPRLookup() + + const outcome = await getPRForBranchOutcome('/remote/repo', MERGED_BRANCH, null, 'ssh-1') + + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + expect(outcome).toMatchObject({ kind: 'upstream-error', errorType: 'unknown' }) + }) + + it('reports an upstream error when the SSH upstream probe loses its provider mid-flight', async () => { + getSshGitProviderMock.mockReturnValue({ + exec: vi.fn().mockRejectedValue(new Error('SSH transport closed')) + }) + mockUpstreamOnlyPRLookup() + + const outcome = await getPRForBranchOutcome('/remote/repo', MERGED_BRANCH, null, 'ssh-1') + + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + expect(outcome).toMatchObject({ kind: 'upstream-error', errorType: 'unknown' }) + }) + + it('reads tracked upstreams through the SSH provider when it is registered', async () => { + const forEachRefArgs = ['for-each-ref', '--format=%(refname)%00%(upstream)', 'refs/heads'] + const sshGitProvider = { + exec: vi.fn(async (args: string[]) => + args[0] === 'for-each-ref' + ? { stdout: `${MERGED_BRANCH}\0origin/contributor/original\n`, stderr: '' } + : { stdout: '', stderr: '' } + ) + } + getSshGitProviderMock.mockReturnValue(sshGitProvider) + mockUpstreamOnlyPRLookup() + + const pr = await getPRForBranch('/remote/repo', MERGED_BRANCH, null, 'ssh-1') + + expect(sshGitProvider.exec).toHaveBeenCalledWith(forEachRefArgs, '/remote/repo') + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + expect(pr).toMatchObject({ number: 78, title: 'Hydrated upstream branch PR' }) + }) + + it('reads tracked upstreams through the local runtime for a WSL repository', async () => { + const forEachRefArgs = ['for-each-ref', '--format=%(refname)%00%(upstream)', 'refs/heads'] + mockUpstreamOnlyPRLookup() + gitExecFileAsyncMock.mockResolvedValue({ + stdout: `${MERGED_BRANCH}\0origin/contributor/original\n`, + stderr: '' + }) + + const pr = await getPRForBranch('/repo-root', MERGED_BRANCH, null, null, null, { + localGitExecOptions: { wslDistro: 'Ubuntu' } + }) + + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(forEachRefArgs, { + cwd: '/repo-root', + wslDistro: 'Ubuntu' + }) + expect(pr).toMatchObject({ number: 78, title: 'Hydrated upstream branch PR' }) + }) +}) diff --git a/src/main/github/client-test-mocks.ts b/src/main/github/client-test-mocks.ts index 7852134552b..15fe0f36214 100644 --- a/src/main/github/client-test-mocks.ts +++ b/src/main/github/client-test-mocks.ts @@ -131,10 +131,18 @@ export function gitRunnerModuleMock(mocks: GitHubClientMocks): GitRunnerModuleMo } } -export type SshGitDispatchModuleMock = { getSshGitProvider: Mock } +export type SshGitDispatchModuleMock = { + getSshGitProviderGeneration: Mock + getSshGitProvider: Mock + SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE: string +} export function sshGitDispatchModuleMock(mocks: GitHubClientMocks): SshGitDispatchModuleMock { - return { getSshGitProvider: mocks.getSshGitProviderMock } + return { + getSshGitProviderGeneration: vi.fn(() => 0), + getSshGitProvider: mocks.getSshGitProviderMock, + SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE: 'Remote connection dropped.' + } } export type LocalGitConfigSignatureModuleMock = { readLocalGitConfigSignature: Mock } diff --git a/src/main/github/client.ts b/src/main/github/client.ts index c3cae80520f..b9e8d94226f 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -73,7 +73,10 @@ import { isCommitPartOfMergedPR, type MergedPRCommitMembership } from './merged-pr-commit-membership' -import { getSshGitProvider } from '../providers/ssh-git-dispatch' +import { + getSshGitProvider, + SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE +} from '../providers/ssh-git-dispatch' import { hasHostedReviewLocalGitOptions, getHostedReviewLocalGitOptions, @@ -2315,14 +2318,19 @@ async function getCurrentHeadOid( connectionId?: string | null, localGitOptions: { wslDistro?: string } = {} ): Promise { + const provider = connectionId ? getSshGitProvider(connectionId) : null + if (connectionId && !provider) { + throw new Error(SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE) + } + if (provider) { + const result = await provider.exec(['rev-parse', 'HEAD'], repoPath) + return result.stdout.trim() || null + } try { - const provider = connectionId ? getSshGitProvider(connectionId) : null - const result = provider - ? await provider.exec(['rev-parse', 'HEAD'], repoPath) - : await gitExecFileAsync(['rev-parse', 'HEAD'], { - cwd: repoPath, - ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}) - }) + const result = await gitExecFileAsync(['rev-parse', 'HEAD'], { + cwd: repoPath, + ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}) + }) return result.stdout.trim() || null } catch { return null @@ -2814,14 +2822,22 @@ async function probeTrackedUpstreamBranches( upstreamsByBranchName: Map }> { const args = ['for-each-ref', '--format=%(refname)%00%(upstream)', 'refs/heads'] + const provider = connectionId ? getSshGitProvider(connectionId) : null + if (connectionId && !provider) { + throw new Error(SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE) + } + if (provider) { + const result = await provider.exec(args, repoPath) + return { + probeFailed: false, + upstreamsByBranchName: parseTrackedUpstreamBranches(result.stdout) + } + } try { - const provider = connectionId ? getSshGitProvider(connectionId) : null - const result = provider - ? await provider.exec(args, repoPath) - : await gitExecFileAsync(args, { - cwd: repoPath, - ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}) - }) + const result = await gitExecFileAsync(args, { + cwd: repoPath, + ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}) + }) return { probeFailed: false, upstreamsByBranchName: parseTrackedUpstreamBranches(result.stdout) diff --git a/src/main/github/gh-utils.ts b/src/main/github/gh-utils.ts index ffeb2ca22c7..7f4c4e05830 100644 --- a/src/main/github/gh-utils.ts +++ b/src/main/github/gh-utils.ts @@ -26,6 +26,7 @@ export { } from './github-repository-identity' export type { GitHubRemoteIdentity, + GitHubRemoteIdentityProbeOptions, GitHubRepoContext, LocalGitExecOptions, OwnerRepo diff --git a/src/main/github/github-api-repository-probe.ts b/src/main/github/github-api-repository-probe.ts new file mode 100644 index 00000000000..08d0c5fd6e3 --- /dev/null +++ b/src/main/github/github-api-repository-probe.ts @@ -0,0 +1,25 @@ +import type { LocalGitExecOptions } from './github-repository-identity' +import { getSshGitProviderGeneration } from '../providers/ssh-git-dispatch' + +export function githubApiRepositoryProbeCacheKey( + repoPath: string, + remoteName: string, + connectionId: string | null | undefined, + localGitOptions: LocalGitExecOptions, + requireVerifiedSshProbe: boolean +): string { + const runtimeKey = connectionId + ? `ssh:${connectionId}:${getSshGitProviderGeneration(connectionId)}` + : `local:${localGitOptions.wslDistro ?? 'host'}` + return `${runtimeKey}\0${repoPath}\0${remoteName}\0${requireVerifiedSshProbe ? 'verified' : 'tolerant'}` +} + +export function resolveGitHubApiRepositoryProbe( + value: T | undefined, + requireVerifiedSshProbe: boolean +): T | null { + if (value === undefined && requireVerifiedSshProbe) { + throw new Error('GitHub repository identity is unverifiable.') + } + return value ?? null +} diff --git a/src/main/github/github-api-repository-validation.ts b/src/main/github/github-api-repository-validation.ts new file mode 100644 index 00000000000..e70197288cd --- /dev/null +++ b/src/main/github/github-api-repository-validation.ts @@ -0,0 +1,20 @@ +import type { GitHubOwnerRepo } from '../../shared/github/pull-request-types' + +export type GitHubApiRepositoryResolution = + | GitHubOwnerRepo + | null + | undefined + | (() => Promise) + +// Why: renderer/RPC overrides reach authenticated REST paths. +const OWNER_SLUG_RE = /^[A-Za-z0-9][A-Za-z0-9-]*$/ +const REPOSITORY_SLUG_RE = /^[A-Za-z0-9._-]+$/ + +export function isValidGitHubApiRepository(repository: GitHubOwnerRepo): boolean { + return ( + OWNER_SLUG_RE.test(repository.owner) && + REPOSITORY_SLUG_RE.test(repository.repo) && + repository.repo !== '.' && + repository.repo !== '..' + ) +} diff --git a/src/main/github/github-api-repository.test.ts b/src/main/github/github-api-repository.test.ts index 2ddfc507630..bf1948588ed 100644 --- a/src/main/github/github-api-repository.test.ts +++ b/src/main/github/github-api-repository.test.ts @@ -1,19 +1,27 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type * as GitHubEnterpriseRepository from './github-enterprise-repository' import type * as GhUtils from './gh-utils' +import type * as SshGitDispatch from '../providers/ssh-git-dispatch' const { getEnterpriseGitHubRepoSlugMock, getOwnerRepoMock, getOwnerRepoForRemoteMock, + getSshGitProviderGenerationMock, isGitHubHostAuthenticatedMock } = vi.hoisted(() => ({ getEnterpriseGitHubRepoSlugMock: vi.fn(), getOwnerRepoMock: vi.fn(), getOwnerRepoForRemoteMock: vi.fn(), + getSshGitProviderGenerationMock: vi.fn(() => 0), isGitHubHostAuthenticatedMock: vi.fn() })) +vi.mock('../providers/ssh-git-dispatch', async (importOriginal) => ({ + ...(await importOriginal()), + getSshGitProviderGeneration: getSshGitProviderGenerationMock +})) + vi.mock('./gh-utils', async (importOriginal) => ({ ...(await importOriginal()), getOwnerRepo: getOwnerRepoMock, @@ -29,6 +37,7 @@ vi.mock('./github-enterprise-repository', async (importOriginal) => ({ import { _resetOriginGitHubApiRepositoryCache, + getGitHubApiRepositoryForRemote, getOriginGitHubApiRepository, githubHostExecOptions, resolveGitHubApiRepository, @@ -40,6 +49,7 @@ beforeEach(() => { getEnterpriseGitHubRepoSlugMock.mockReset().mockResolvedValue(null) getOwnerRepoMock.mockReset().mockResolvedValue(null) getOwnerRepoForRemoteMock.mockReset().mockResolvedValue(null) + getSshGitProviderGenerationMock.mockReset().mockReturnValue(0) isGitHubHostAuthenticatedMock.mockReset().mockResolvedValue(false) }) @@ -182,6 +192,62 @@ describe('resolveGitHubRepoExecution', () => { }) describe('origin repository cache', () => { + it('isolates Enterprise identity across SSH provider generations', async () => { + const beforeReconnect = { owner: 'acme', repo: 'widgets', host: 'github.acme-corp.com' } + const afterReconnect = { owner: 'acme', repo: 'other', host: 'github.acme-corp.com' } + getEnterpriseGitHubRepoSlugMock + .mockResolvedValueOnce(beforeReconnect) + .mockResolvedValueOnce(afterReconnect) + + await expect(getOriginGitHubApiRepository('/remote/repo', 'ssh-1')).resolves.toEqual( + beforeReconnect + ) + getSshGitProviderGenerationMock.mockReturnValue(1) + await expect(getOriginGitHubApiRepository('/remote/repo', 'ssh-1')).resolves.toEqual( + afterReconnect + ) + expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(2) + }) + + it('keeps an indeterminate auth inventory unverifiable during candidate discovery', async () => { + getEnterpriseGitHubRepoSlugMock.mockResolvedValue(undefined) + + await expect( + getGitHubApiRepositoryForRemote( + '/remote/repo', + 'origin', + 'ssh-1', + {}, + { + requireVerifiedSshProbe: true + } + ) + ).rejects.toThrow('GitHub repository identity is unverifiable.') + }) + + it('does not reuse a tolerant SSH miss for verified candidate discovery', async () => { + const enterprise = { + owner: 'acme', + repo: 'widgets', + host: 'github.acme-corp.com' + } + getEnterpriseGitHubRepoSlugMock.mockResolvedValueOnce(null).mockResolvedValueOnce(enterprise) + + await expect(getOriginGitHubApiRepository('/remote/repo', 'ssh-1')).resolves.toBeNull() + await expect( + getGitHubApiRepositoryForRemote( + '/remote/repo', + 'origin', + 'ssh-1', + {}, + { + requireVerifiedSshProbe: true + } + ) + ).resolves.toEqual(enterprise) + expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(2) + }) + it('does not cache an indeterminate Enterprise auth probe', async () => { const enterprise = { owner: 'acme', diff --git a/src/main/github/github-api-repository.ts b/src/main/github/github-api-repository.ts index d7fabcd4f5b..7e394789714 100644 --- a/src/main/github/github-api-repository.ts +++ b/src/main/github/github-api-repository.ts @@ -8,6 +8,7 @@ import { getOwnerRepoForRemote, ghRepoExecOptions, githubRepoContext, + type GitHubRemoteIdentityProbeOptions, type LocalGitExecOptions } from './gh-utils' import { @@ -15,7 +16,21 @@ import { getEnterpriseGitHubRepoSlugForRemote, isGitHubHostAuthenticated } from './github-enterprise-repository' +import { githubHostExecOptions } from './github-repository-host' +import { + isValidGitHubApiRepository, + type GitHubApiRepositoryResolution +} from './github-api-repository-validation' +import { + githubApiRepositoryProbeCacheKey, + resolveGitHubApiRepositoryProbe +} from './github-api-repository-probe' +export { + githubHostExecOptions, + githubRepositorySlugArg, + githubRepositoryWebHost +} from './github-repository-host' export type GitHubApiRepository = GitHubOwnerRepo export type GitHubRepoExecOptions = ReturnType & { host?: string } export type GitHubRepoExecution = { @@ -23,43 +38,12 @@ export type GitHubRepoExecution = { ghOptions: GitHubRepoExecOptions } -type GitHubApiRepositoryResolution = - | GitHubApiRepository - | null - | undefined - | (() => Promise) - -// Why: renderer/RPC repository overrides are interpolated into REST paths. -// Reject path syntax before an authenticated gh process can target it. -const GITHUB_OWNER_SLUG_RE = /^[A-Za-z0-9][A-Za-z0-9-]*$/ -const GITHUB_REPO_SLUG_RE = /^[A-Za-z0-9._-]+$/ - -function isValidGitHubApiRepository(repository: GitHubApiRepository): boolean { - return ( - GITHUB_OWNER_SLUG_RE.test(repository.owner) && - GITHUB_REPO_SLUG_RE.test(repository.repo) && - repository.repo !== '.' && - repository.repo !== '..' - ) -} - -// Why: the enterprise branch spawns an uncached `git remote get-url` (an SSH -// round trip on connection-backed repos) — hot paths like per-file contents -// and viewed-state toggles resolve per call, so cache like ownerRepoCache does. +// Why: cache the uncached Enterprise remote probe used by hot paths. const ORIGIN_REPO_CACHE_TTL_MS = 30_000 const ORIGIN_REPO_CACHE_MAX_ENTRIES = 512 const originRepoCache = new Map() const originRepoInFlight = new Map>() -function originRepoCacheKey( - repoPath: string, - remoteName: string, - connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} -): string { - return `${connectionId ?? 'local'}\0${localGitOptions.wslDistro ?? ''}\0${repoPath}\0${remoteName}` -} - /** @internal - exposed for tests only */ export function _resetOriginGitHubApiRepositoryCache(): void { originRepoCache.clear() @@ -90,15 +74,30 @@ export async function getGitHubApiRepositoryForRemote( repoPath: string, remoteName: string, connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} + localGitOptions: LocalGitExecOptions = {}, + probeOptions: GitHubRemoteIdentityProbeOptions = {} ): Promise { // Why: generic PR resolution prefers upstream, but this API represents the // caller-selected remote exactly (#7331). - const ownerRepo = await getOwnerRepoForRemote(repoPath, remoteName, connectionId, localGitOptions) + const requireVerifiedSshProbe = probeOptions.requireVerifiedSshProbe === true + const verifiedIdentityArgs = requireVerifiedSshProbe ? ([probeOptions] as const) : [] + const ownerRepo = await getOwnerRepoForRemote( + repoPath, + remoteName, + connectionId, + localGitOptions, + ...verifiedIdentityArgs + ) if (ownerRepo) { return { ...ownerRepo, host: 'github.com' } } - const cacheKey = originRepoCacheKey(repoPath, remoteName, connectionId, localGitOptions) + const cacheKey = githubApiRepositoryProbeCacheKey( + repoPath, + remoteName, + connectionId, + localGitOptions, + requireVerifiedSshProbe + ) const now = Date.now() pruneOriginRepoCache(now) const cached = originRepoCache.get(cacheKey) @@ -112,14 +111,21 @@ export async function getGitHubApiRepositoryForRemote( const probe = (async () => { const enterpriseOptions = Object.keys(localGitOptions).length > 0 ? { localGitExecOptions: localGitOptions } : {} + const verifiedEnterpriseArgs = requireVerifiedSshProbe ? ([true] as const) : [] const slug = remoteName === 'origin' - ? await getEnterpriseGitHubRepoSlug(repoPath, connectionId, enterpriseOptions) + ? await getEnterpriseGitHubRepoSlug( + repoPath, + connectionId, + enterpriseOptions, + ...verifiedEnterpriseArgs + ) : await getEnterpriseGitHubRepoSlugForRemote( repoPath, remoteName, connectionId, - enterpriseOptions + enterpriseOptions, + ...verifiedEnterpriseArgs ) // Why: undefined means the gh auth inventory could not be read. Caching it // as a negative would turn a transient spawn failure into a 30-second miss. @@ -130,7 +136,7 @@ export async function getGitHubApiRepositoryForRemote( }) pruneOriginRepoCache(Date.now()) } - return slug ?? null + return resolveGitHubApiRepositoryProbe(slug, requireVerifiedSshProbe) })() originRepoInFlight.set(cacheKey, probe) try { @@ -180,8 +186,12 @@ export async function resolveGitHubApiRepositoryCandidates( localGitOptions: LocalGitExecOptions = {} ): Promise { const [upstream, origin] = await Promise.all([ - getGitHubApiRepositoryForRemote(repoPath, 'upstream', connectionId, localGitOptions), - getGitHubApiRepositoryForRemote(repoPath, 'origin', connectionId, localGitOptions) + getGitHubApiRepositoryForRemote(repoPath, 'upstream', connectionId, localGitOptions, { + requireVerifiedSshProbe: true + }), + getGitHubApiRepositoryForRemote(repoPath, 'origin', connectionId, localGitOptions, { + requireVerifiedSshProbe: true + }) ]) const seen = new Set() const candidates: GitHubApiRepository[] = [] @@ -292,15 +302,6 @@ export async function resolveGitHubApiRepository( return null } -// Why: the gh runner host-qualifies argv from `options.host`, so every known -// host must be carried through. Pinning github.com prevents a process-level -// GH_HOST from silently redirecting an otherwise unambiguous API request. -export function githubHostExecOptions(repository: GitHubApiRepository | null | undefined): { - host?: string -} { - return repository?.host ? { host: repository.host } : {} -} - export async function resolveGitHubRepoExecution( repoPath: string, repository?: GitHubApiRepositoryResolution, @@ -329,19 +330,3 @@ export async function resolveGitHubRepoExecution( } } } - -export function githubRepositoryWebHost(repository: GitHubApiRepository): string { - return repository.host ?? 'github.com' -} - -/** - * Positional `HOST/OWNER/REPO` argv value (e.g. `gh repo view `). - * Positional slugs bypass the runner's `--repo` qualifier, so they must be - * qualified here whenever the host is known. - */ -export function githubRepositorySlugArg(repository: GitHubApiRepository): string { - const slug = `${repository.owner}/${repository.repo}` - // Why: github.com must be explicit too; otherwise process-level GH_HOST can - // redirect positional `gh repo view OWNER/REPO` calls to an Enterprise host. - return repository.host ? `${repository.host}/${slug}` : slug -} diff --git a/src/main/github/github-enterprise-repository.ts b/src/main/github/github-enterprise-repository.ts index f91975097fc..0d244039145 100644 --- a/src/main/github/github-enterprise-repository.ts +++ b/src/main/github/github-enterprise-repository.ts @@ -18,6 +18,11 @@ import { } from './github-remote-identity-parsing' import { resolveSshConfigHostname } from './github-ssh-host-alias-resolution' import { parseWslPath } from '../wsl' +import { + getSshGitProvider, + SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE +} from '../providers/ssh-git-dispatch' +import { isStableMissingGitRemoteError } from '../git/stable-missing-git-remote-error' export type GitHubEnterpriseRepoSlug = GitHubOwnerRepo & { host: string } @@ -214,16 +219,26 @@ export async function getEnterpriseGitHubRepoSlugForRemote( repoPath: string, remoteName: string, connectionId?: string | null, - options: HostedReviewExecutionOptions = {} + options: HostedReviewExecutionOptions = {}, + requireVerifiedSshProbe = false ): Promise { const localGitOptions = getHostedReviewLocalGitOptions(options) const context = githubRepoContext(repoPath, connectionId, localGitOptions) + if (requireVerifiedSshProbe && connectionId && !getSshGitProvider(connectionId)) { + throw new Error(SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE) + } let remoteUrl: string | null try { remoteUrl = await getRemoteUrlForRepo(context, remoteName) - } catch { + } catch (error) { + if (requireVerifiedSshProbe && connectionId && !isStableMissingGitRemoteError(error)) { + throw error + } return null } + if (requireVerifiedSshProbe && connectionId && !remoteUrl && !getSshGitProvider(connectionId)) { + throw new Error(SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE) + } const identity = remoteUrl ? parseGitHubRemoteIdentity(remoteUrl) : null if (!identity) { return null @@ -234,6 +249,9 @@ export async function getEnterpriseGitHubRepoSlugForRemote( if (aliasHost) { const { hostname, resolved } = await resolveSshConfigHostname(aliasHost, context) if (!resolved || !hostname) { + if (requireVerifiedSshProbe && connectionId) { + throw new Error('Remote repository identity is unverifiable.') + } const authenticatedLiteralHost = await resolveAuthenticatedGitHubHost( identity.host, repoPath, @@ -266,7 +284,14 @@ export async function getEnterpriseGitHubRepoSlugForRemote( export async function getEnterpriseGitHubRepoSlug( repoPath: string, connectionId?: string | null, - options: HostedReviewExecutionOptions = {} + options: HostedReviewExecutionOptions = {}, + requireVerifiedSshProbe = false ): Promise { - return getEnterpriseGitHubRepoSlugForRemote(repoPath, 'origin', connectionId, options) + return getEnterpriseGitHubRepoSlugForRemote( + repoPath, + 'origin', + connectionId, + options, + requireVerifiedSshProbe + ) } diff --git a/src/main/github/github-repository-host.ts b/src/main/github/github-repository-host.ts new file mode 100644 index 00000000000..250f71d38c3 --- /dev/null +++ b/src/main/github/github-repository-host.ts @@ -0,0 +1,18 @@ +import type { GitHubOwnerRepo } from '../../shared/github/pull-request-types' + +export function githubHostExecOptions(repository: GitHubOwnerRepo | null | undefined): { + host?: string +} { + return repository?.host ? { host: repository.host } : {} +} + +export function githubRepositoryWebHost(repository: GitHubOwnerRepo): string { + return repository.host ?? 'github.com' +} + +/** Host-qualified positional slug for commands that bypass the runner's `--repo`. */ +export function githubRepositorySlugArg(repository: GitHubOwnerRepo): string { + const slug = `${repository.owner}/${repository.repo}` + // Why: pin dotcom too so process-level GH_HOST cannot redirect the request. + return repository.host ? `${repository.host}/${slug}` : slug +} diff --git a/src/main/github/github-repository-identity.ssh-host-alias.test.ts b/src/main/github/github-repository-identity.ssh-host-alias.test.ts index 26ad3504bfa..d08d5335f6e 100644 --- a/src/main/github/github-repository-identity.ssh-host-alias.test.ts +++ b/src/main/github/github-repository-identity.ssh-host-alias.test.ts @@ -25,7 +25,8 @@ vi.mock('../git/runner', async (importOriginal) => ({ vi.mock('../providers/ssh-git-dispatch', () => ({ getSshGitProvider: getSshGitProviderMock, - getSshGitProviderGeneration: getSshGitProviderGenerationMock + getSshGitProviderGeneration: getSshGitProviderGenerationMock, + SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE: 'Remote connection dropped.' })) vi.mock('./local-git-config-signature', () => ({ @@ -174,6 +175,70 @@ describe('#10284 SSH Host alias → github.com owner/repo', () => { expect(resolveWithSshGMock).not.toHaveBeenCalled() }) + it('does not read an SSH repoPath locally when its provider is missing', async () => { + getSshGitProviderMock.mockReturnValue(undefined) + + await expect( + getOwnerRepoForRemote( + '/remote/repo', + 'origin', + 'ssh-1', + {}, + { + requireVerifiedSshProbe: true + } + ) + ).rejects.toThrow('Remote connection dropped.') + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('preserves a one-shot SSH remote failure through candidate discovery', async () => { + const failure = new Error('relay request failed') + const provider = { + exec: vi + .fn() + .mockRejectedValueOnce(failure) + .mockResolvedValueOnce({ stdout: 'git@github.com:team/orca.git\n', stderr: '' }) + } + getSshGitProviderMock.mockReturnValue(provider) + + await expect( + getOwnerRepoForRemote( + '/remote/repo', + 'origin', + 'ssh-1', + {}, + { + requireVerifiedSshProbe: true + } + ) + ).rejects.toBe(failure) + await expect(getOwnerRepoForRemote('/remote/repo', 'origin', 'ssh-1')).resolves.toEqual({ + owner: 'team', + repo: 'orca' + }) + expect(provider.exec).toHaveBeenCalledTimes(2) + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('keeps failed SSH alias classification unverifiable', async () => { + const provider = sshProvider('') + getSshGitProviderMock.mockReturnValue(provider) + + await expect( + getOwnerRepoForRemote( + '/remote/repo', + 'origin', + 'ssh-1', + {}, + { + requireVerifiedSshProbe: true + } + ) + ).rejects.toThrow('Remote repository identity is unverifiable.') + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + it('isolates the same alias across native and WSL runtimes', async () => { resolveWithSshGMock.mockResolvedValueOnce(sshConfig('github.com')) commandExecFileAsyncMock.mockResolvedValueOnce({ diff --git a/src/main/github/github-repository-identity.ts b/src/main/github/github-repository-identity.ts index b860c7b7d31..c912f424417 100644 --- a/src/main/github/github-repository-identity.ts +++ b/src/main/github/github-repository-identity.ts @@ -1,7 +1,11 @@ import { runCoalescedProbe, type CoalescedProbes } from '../git/coalesced-probe' import { readRemoteUrl } from '../git/remote-url-probe' import type { GitHubOwnerRepo } from '../../shared/github/pull-request-types' -import { getSshGitProviderGeneration } from '../providers/ssh-git-dispatch' +import { + getSshGitProvider, + getSshGitProviderGeneration, + SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE +} from '../providers/ssh-git-dispatch' import { readLocalGitConfigSignature } from './local-git-config-signature' import { parseGitHubOwnerRepo, @@ -26,6 +30,10 @@ export type LocalGitExecOptions = { wslDistro?: string } +export type GitHubRemoteIdentityProbeOptions = { + requireVerifiedSshProbe?: boolean +} + export function githubRepoContext( repoPath: string, connectionId?: string | null, @@ -108,9 +116,17 @@ export async function getOwnerRepoForRemote( repoPath: string, remoteName: string, connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} + localGitOptions: LocalGitExecOptions = {}, + probeOptions: GitHubRemoteIdentityProbeOptions = {} ): Promise { const context = githubRepoContext(repoPath, connectionId, localGitOptions) + if ( + probeOptions.requireVerifiedSshProbe && + context.connectionId && + !getSshGitProvider(context.connectionId) + ) { + throw new Error(SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE) + } const runtimeKey = context.connectionId ? `ssh:${context.connectionId}:${getSshGitProviderGeneration(context.connectionId)}` : `local:${context.wslDistro ?? 'host'}` @@ -145,8 +161,15 @@ export async function getOwnerRepoForRemote( // for the same repo concurrently. Coalesce missing-remote probes — but only // onto one young enough to still answer, so a wedged probe cannot pin the // repo's identity for the life of the process (P1-D). - return runCoalescedProbe(ownerRepoInFlight, cacheKey, () => - resolveOwnerRepoForRemote(context, remoteName, cacheKey, nextConfigSignature) + const inFlightKey = `${cacheKey}\0${probeOptions.requireVerifiedSshProbe ? 'verified' : 'tolerant'}` + return runCoalescedProbe(ownerRepoInFlight, inFlightKey, () => + resolveOwnerRepoForRemote( + context, + remoteName, + cacheKey, + nextConfigSignature, + probeOptions.requireVerifiedSshProbe === true + ) ) } @@ -154,12 +177,20 @@ async function resolveOwnerRepoForRemote( context: GitHubRepoContext, remoteName: string, cacheKey: string, - configSignature?: string + configSignature: string | undefined, + requireVerifiedSshProbe: boolean ): Promise { const now = Date.now() try { const remoteUrl = await getRemoteUrlForRepo(context, remoteName) if (!remoteUrl) { + if ( + requireVerifiedSshProbe && + context.connectionId && + !getSshGitProvider(context.connectionId) + ) { + throw new Error(SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE) + } // Empty remote URL is stable until git config changes. ownerRepoCache.set(cacheKey, { value: null, @@ -181,6 +212,9 @@ async function resolveOwnerRepoForRemote( } if (classification.kind === 'indeterminate') { // Why: a failed ssh -G probe is not a stable "not GitHub" result. + if (requireVerifiedSshProbe && context.connectionId) { + throw new Error('Remote repository identity is unverifiable.') + } return null } const stableConfigSignature = classification.cacheWithGitConfigSignature @@ -197,6 +231,9 @@ async function resolveOwnerRepoForRemote( // Why: only stable "no such remote" misses are safe to hold for minutes. // Transient git lock/IO failures must retry on the next lookup. if (!isStableMissingGitRemoteError(error)) { + if (requireVerifiedSshProbe && context.connectionId) { + throw error + } return null } }