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..a7dc26f4fdf --- /dev/null +++ b/src/main/github/github-api-repository-validation.ts @@ -0,0 +1,14 @@ +import type { GitHubOwnerRepo } from '../../shared/github/pull-request-types' + +// 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..7aa89984cbe 100644 --- a/src/main/github/github-api-repository.test.ts +++ b/src/main/github/github-api-repository.test.ts @@ -29,6 +29,7 @@ vi.mock('./github-enterprise-repository', async (importOriginal) => ({ import { _resetOriginGitHubApiRepositoryCache, + getGitHubApiRepositoryForRemote, getOriginGitHubApiRepository, githubHostExecOptions, resolveGitHubApiRepository, @@ -182,6 +183,29 @@ describe('resolveGitHubRepoExecution', () => { }) describe('origin repository cache', () => { + 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..864d32f97b1 100644 --- a/src/main/github/github-api-repository.ts +++ b/src/main/github/github-api-repository.ts @@ -15,6 +15,14 @@ import { getEnterpriseGitHubRepoSlugForRemote, isGitHubHostAuthenticated } from './github-enterprise-repository' +import { githubHostExecOptions } from './github-repository-host' +import { isValidGitHubApiRepository } from './github-api-repository-validation' + +export { + githubHostExecOptions, + githubRepositorySlugArg, + githubRepositoryWebHost +} from './github-repository-host' export type GitHubApiRepository = GitHubOwnerRepo export type GitHubRepoExecOptions = ReturnType & { host?: string } @@ -23,26 +31,16 @@ export type GitHubRepoExecution = { ghOptions: GitHubRepoExecOptions } +type GitHubApiRepositoryProbeOptions = { + requireVerifiedSshProbe?: boolean +} + 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. @@ -55,9 +53,10 @@ function originRepoCacheKey( repoPath: string, remoteName: string, connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} + localGitOptions: LocalGitExecOptions = {}, + requireVerifiedSshProbe = false ): string { - return `${connectionId ?? 'local'}\0${localGitOptions.wslDistro ?? ''}\0${repoPath}\0${remoteName}` + return `${connectionId ?? 'local'}\0${localGitOptions.wslDistro ?? ''}\0${repoPath}\0${remoteName}\0${requireVerifiedSshProbe ? 'verified' : 'tolerant'}` } /** @internal - exposed for tests only */ @@ -90,15 +89,28 @@ export async function getGitHubApiRepositoryForRemote( repoPath: string, remoteName: string, connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} + localGitOptions: LocalGitExecOptions = {}, + probeOptions: GitHubApiRepositoryProbeOptions = {} ): 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 ownerRepo = await getOwnerRepoForRemote( + repoPath, + remoteName, + connectionId, + localGitOptions, + probeOptions + ) if (ownerRepo) { return { ...ownerRepo, host: 'github.com' } } - const cacheKey = originRepoCacheKey(repoPath, remoteName, connectionId, localGitOptions) + const cacheKey = originRepoCacheKey( + repoPath, + remoteName, + connectionId, + localGitOptions, + probeOptions.requireVerifiedSshProbe === true + ) const now = Date.now() pruneOriginRepoCache(now) const cached = originRepoCache.get(cacheKey) @@ -114,12 +126,18 @@ export async function getGitHubApiRepositoryForRemote( Object.keys(localGitOptions).length > 0 ? { localGitExecOptions: localGitOptions } : {} const slug = remoteName === 'origin' - ? await getEnterpriseGitHubRepoSlug(repoPath, connectionId, enterpriseOptions) + ? await getEnterpriseGitHubRepoSlug( + repoPath, + connectionId, + enterpriseOptions, + probeOptions.requireVerifiedSshProbe === true + ) : await getEnterpriseGitHubRepoSlugForRemote( repoPath, remoteName, connectionId, - enterpriseOptions + enterpriseOptions, + probeOptions.requireVerifiedSshProbe === true ) // 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. @@ -180,8 +198,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 +314,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 +342,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 c0d11650a95..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 @@ -178,9 +178,17 @@ describe('#10284 SSH Host alias → github.com owner/repo', () => { 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')).rejects.toThrow( - 'Remote connection dropped.' - ) + await expect( + getOwnerRepoForRemote( + '/remote/repo', + 'origin', + 'ssh-1', + {}, + { + requireVerifiedSshProbe: true + } + ) + ).rejects.toThrow('Remote connection dropped.') expect(gitExecFileAsyncMock).not.toHaveBeenCalled() }) @@ -194,7 +202,17 @@ describe('#10284 SSH Host alias → github.com owner/repo', () => { } getSshGitProviderMock.mockReturnValue(provider) - await expect(getOwnerRepoForRemote('/remote/repo', 'origin', 'ssh-1')).rejects.toBe(failure) + 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' @@ -207,9 +225,17 @@ describe('#10284 SSH Host alias → github.com owner/repo', () => { const provider = sshProvider('') getSshGitProviderMock.mockReturnValue(provider) - await expect(getOwnerRepoForRemote('/remote/repo', 'origin', 'ssh-1')).rejects.toThrow( - 'Remote repository identity is unverifiable.' - ) + await expect( + getOwnerRepoForRemote( + '/remote/repo', + 'origin', + 'ssh-1', + {}, + { + requireVerifiedSshProbe: true + } + ) + ).rejects.toThrow('Remote repository identity is unverifiable.') expect(gitExecFileAsyncMock).not.toHaveBeenCalled() }) diff --git a/src/main/github/github-repository-identity.ts b/src/main/github/github-repository-identity.ts index 5f2f454f85d..c912f424417 100644 --- a/src/main/github/github-repository-identity.ts +++ b/src/main/github/github-repository-identity.ts @@ -30,6 +30,10 @@ export type LocalGitExecOptions = { wslDistro?: string } +export type GitHubRemoteIdentityProbeOptions = { + requireVerifiedSshProbe?: boolean +} + export function githubRepoContext( repoPath: string, connectionId?: string | null, @@ -112,10 +116,15 @@ 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 (context.connectionId && !getSshGitProvider(context.connectionId)) { + if ( + probeOptions.requireVerifiedSshProbe && + context.connectionId && + !getSshGitProvider(context.connectionId) + ) { throw new Error(SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE) } const runtimeKey = context.connectionId @@ -152,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 + ) ) } @@ -161,13 +177,18 @@ 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 (context.connectionId && !getSshGitProvider(context.connectionId)) { + if ( + requireVerifiedSshProbe && + context.connectionId && + !getSshGitProvider(context.connectionId) + ) { throw new Error(SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE) } // Empty remote URL is stable until git config changes. @@ -191,7 +212,7 @@ async function resolveOwnerRepoForRemote( } if (classification.kind === 'indeterminate') { // Why: a failed ssh -G probe is not a stable "not GitHub" result. - if (context.connectionId) { + if (requireVerifiedSshProbe && context.connectionId) { throw new Error('Remote repository identity is unverifiable.') } return null @@ -210,7 +231,7 @@ 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 (context.connectionId) { + if (requireVerifiedSshProbe && context.connectionId) { throw error } return null