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 } }