fix(github): scope verified SSH identity probes

This commit is contained in:
Brennan Benson
2026-08-16 20:12:53 -07:00
parent 70cb101736
commit eb80807db5
7 changed files with 192 additions and 67 deletions
@@ -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 !== '..'
)
}
@@ -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',
+45 -48
View File
@@ -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<typeof ghRepoExecOptions> & { host?: string }
@@ -23,26 +31,16 @@ export type GitHubRepoExecution = {
ghOptions: GitHubRepoExecOptions
}
type GitHubApiRepositoryProbeOptions = {
requireVerifiedSshProbe?: boolean
}
type GitHubApiRepositoryResolution =
| GitHubApiRepository
| null
| undefined
| (() => Promise<GitHubApiRepository | null>)
// 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<GitHubApiRepository | null> {
// 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<GitHubApiRepositoryCandidates> {
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<string>()
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 <slug>`).
* 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
}
@@ -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<GitHubEnterpriseRepoSlug | null | undefined> {
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<GitHubEnterpriseRepoSlug | null | undefined> {
return getEnterpriseGitHubRepoSlugForRemote(repoPath, 'origin', connectionId, options)
return getEnterpriseGitHubRepoSlugForRemote(
repoPath,
'origin',
connectionId,
options,
requireVerifiedSshProbe
)
}
+18
View File
@@ -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
}
@@ -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()
})
+29 -8
View File
@@ -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<OwnerRepo | null> {
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<OwnerRepo | null> {
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