diff --git a/src/main/github/github-api-repository.test.ts b/src/main/github/github-api-repository.test.ts index bf1948588ed..fb6512dab30 100644 --- a/src/main/github/github-api-repository.test.ts +++ b/src/main/github/github-api-repository.test.ts @@ -1,7 +1,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as SshGitDispatch from '../providers/ssh-git-dispatch' 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, @@ -268,4 +268,81 @@ describe('origin repository cache', () => { await expect(getOriginGitHubApiRepository('/repo')).resolves.toBeNull() expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(1) }) + + it('does not reuse a cached answer after the SSH provider reconnects', async () => { + const oldRepository = { + owner: 'old-owner', + repo: 'widgets', + host: 'github.acme-corp.com' + } + const newRepository = { + owner: 'new-owner', + repo: 'widgets', + host: 'github.acme-corp.com' + } + getSshGitProviderGenerationMock.mockReturnValue(1) + getEnterpriseGitHubRepoSlugMock + .mockResolvedValueOnce(oldRepository) + .mockResolvedValueOnce(newRepository) + + await expect(getOriginGitHubApiRepository('/repo', 'ssh-1')).resolves.toEqual(oldRepository) + await expect(getOriginGitHubApiRepository('/repo', 'ssh-1')).resolves.toEqual(oldRepository) + expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(1) + + getSshGitProviderGenerationMock.mockReturnValue(2) + await expect(getOriginGitHubApiRepository('/repo', 'ssh-1')).resolves.toEqual(newRepository) + expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(2) + }) + + it('does not reuse a cached negative after the SSH provider reconnects', async () => { + const repository = { + owner: 'acme', + repo: 'widgets', + host: 'github.acme-corp.com' + } + getSshGitProviderGenerationMock.mockReturnValue(1) + getEnterpriseGitHubRepoSlugMock.mockResolvedValueOnce(null).mockResolvedValueOnce(repository) + + await expect(getOriginGitHubApiRepository('/repo', 'ssh-1')).resolves.toBeNull() + await expect(getOriginGitHubApiRepository('/repo', 'ssh-1')).resolves.toBeNull() + expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(1) + + getSshGitProviderGenerationMock.mockReturnValue(2) + await expect(getOriginGitHubApiRepository('/repo', 'ssh-1')).resolves.toEqual(repository) + expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(2) + }) + + it('isolates an in-flight probe from an older SSH provider generation', async () => { + const oldRepository = { + owner: 'old-owner', + repo: 'widgets', + host: 'github.acme-corp.com' + } + const newRepository = { + owner: 'new-owner', + repo: 'widgets', + host: 'github.acme-corp.com' + } + let resolveOldProbe: (repository: typeof oldRepository) => void = () => {} + const oldProbeResult = new Promise((resolve) => { + resolveOldProbe = resolve + }) + getEnterpriseGitHubRepoSlugMock + .mockImplementationOnce(() => oldProbeResult) + .mockResolvedValueOnce(newRepository) + getSshGitProviderGenerationMock.mockReturnValue(1) + + const oldProbe = getOriginGitHubApiRepository('/repo', 'ssh-1') + await vi.waitFor(() => expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(1)) + + getSshGitProviderGenerationMock.mockReturnValue(2) + const newProbe = getOriginGitHubApiRepository('/repo', 'ssh-1') + await vi.waitFor(() => expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(2)) + await expect(newProbe).resolves.toEqual(newRepository) + + resolveOldProbe(oldRepository) + await expect(oldProbe).resolves.toEqual(oldRepository) + await expect(getOriginGitHubApiRepository('/repo', 'ssh-1')).resolves.toEqual(newRepository) + expect(getEnterpriseGitHubRepoSlugMock).toHaveBeenCalledTimes(2) + }) }) diff --git a/src/main/github/github-enterprise-repository.test.ts b/src/main/github/github-enterprise-repository.test.ts index caee8e0bccf..dac93c146e0 100644 --- a/src/main/github/github-enterprise-repository.test.ts +++ b/src/main/github/github-enterprise-repository.test.ts @@ -27,6 +27,7 @@ import { isGitHubHostAuthenticatedForGlobalCli } from './github-enterprise-repository' import { _resetSshHostnameResolutionCache } from './github-ssh-host-alias-resolution' +import { registerSshGitProvider, unregisterSshGitProvider } from '../providers/ssh-git-dispatch' function mockOriginRemote(url: string): void { gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { @@ -392,15 +393,122 @@ describe('isGitHubHostAuthenticated', () => { } }) - it('shares the native gh auth probe across SSH connections', async () => { - mockHostAuthenticated() + it('does not share a cached answer between SSH connections at the same path', async () => { + ghExecFileAsyncMock + .mockResolvedValueOnce({ + stdout: + 'github.acme-corp.com\n ✓ Logged in to github.acme-corp.com account kelora (keyring)', + stderr: '' + }) + .mockResolvedValueOnce({ + stdout: 'github.com\n ✓ Logged in to github.com account kelora (keyring)', + stderr: '' + }) - await isGitHubHostAuthenticated('github.acme-corp.com', '/remote/a', 'ssh-1') - await isGitHubHostAuthenticated('github.acme-corp.com', '/remote/b', 'ssh-2') - - expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + await expect(isGitHubHostAuthenticated('github.acme-corp.com', '/repo', 'ssh-1')).resolves.toBe( + true + ) + await expect(isGitHubHostAuthenticated('github.acme-corp.com', '/repo', 'ssh-2')).resolves.toBe( + false + ) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) }) + it('does not reuse a cached answer after the SSH provider reconnects', async () => { + const connectionId = 'ssh-reconnected-auth-cache' + ghExecFileAsyncMock + .mockResolvedValueOnce({ + stdout: + 'github.acme-corp.com\n ✓ Logged in to github.acme-corp.com account kelora (keyring)', + stderr: '' + }) + .mockResolvedValueOnce({ + stdout: 'github.com\n ✓ Logged in to github.com account kelora (keyring)', + stderr: '' + }) + + registerSshGitProvider(connectionId, {} as never) + try { + await expect( + isGitHubHostAuthenticated('github.acme-corp.com', '/repo', connectionId) + ).resolves.toBe(true) + await expect( + isGitHubHostAuthenticated('github.acme-corp.com', '/repo', connectionId) + ).resolves.toBe(true) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + + registerSshGitProvider(connectionId, {} as never) + await expect( + isGitHubHostAuthenticated('github.acme-corp.com', '/repo', connectionId) + ).resolves.toBe(false) + } finally { + unregisterSshGitProvider(connectionId) + } + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('does not join an in-flight auth probe from an older SSH provider generation', async () => { + const connectionId = 'ssh-reconnected-auth-in-flight' + let finishOldProbe: (() => void) | undefined + ghExecFileAsyncMock + .mockImplementationOnce( + () => + new Promise((resolve) => { + finishOldProbe = () => + resolve({ + stdout: + 'github.acme-corp.com\n ✓ Logged in to github.acme-corp.com account kelora (keyring)', + stderr: '' + }) + }) + ) + .mockResolvedValueOnce({ + stdout: 'github.com\n ✓ Logged in to github.com account kelora (keyring)', + stderr: '' + }) + + registerSshGitProvider(connectionId, {} as never) + try { + const oldProbe = isGitHubHostAuthenticated('github.acme-corp.com', '/repo', connectionId) + registerSshGitProvider(connectionId, {} as never) + const newProbe = isGitHubHostAuthenticated('github.acme-corp.com', '/repo', connectionId) + + await expect(newProbe).resolves.toBe(false) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + finishOldProbe?.() + await expect(oldProbe).resolves.toBe(true) + } finally { + unregisterSshGitProvider(connectionId) + } + }) + + // The probe answers differ by runtime: a local repo gets a cwd, a + // connection-backed repo does not. Sharing one entry attributes whichever + // resolved first to both hosts. + it.each([ + ['local resolves first', [null, 'ssh-1'] as const, [true, false]], + ['connection resolves first', ['ssh-1', null] as const, [false, true]] + ])( + 'does not cross-attribute auth between a local and a connection-backed repo at the same path (%s)', + async (_label, connectionIds, expected) => { + ghExecFileAsyncMock.mockImplementation( + async (_args: string[], options: { cwd?: string }) => ({ + stdout: options.cwd + ? 'github.acme-corp.com\n ✓ Logged in to github.acme-corp.com account kelora (keyring)' + : 'github.com\n ✓ Logged in to github.com account kelora (keyring)', + stderr: '' + }) + ) + + for (const [index, connectionId] of connectionIds.entries()) { + await expect( + isGitHubHostAuthenticated('github.acme-corp.com', '/repo', connectionId) + ).resolves.toBe(expected[index]) + } + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + } + ) + it('does not target an unconfigured remote host with ambient credentials', async () => { mockHostAuthenticated('github.acme-corp.com') diff --git a/src/main/github/github-enterprise-repository.ts b/src/main/github/github-enterprise-repository.ts index 0d244039145..d32dfadb3b9 100644 --- a/src/main/github/github-enterprise-repository.ts +++ b/src/main/github/github-enterprise-repository.ts @@ -20,7 +20,8 @@ import { resolveSshConfigHostname } from './github-ssh-host-alias-resolution' import { parseWslPath } from '../wsl' import { getSshGitProvider, - SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE + SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE, + getSshGitProviderGeneration } from '../providers/ssh-git-dispatch' import { isStableMissingGitRemoteError } from '../git/stable-missing-git-remote-error' @@ -41,9 +42,19 @@ type HostAuthCacheEntry = { const hostAuthCache = new Map() const hostAuthInFlight = new Map>() -// Why: connection-backed Git operations execute remotely, but gh intentionally -// executes on the native host. Only WSL selects a distinct gh config/runtime. -function runtimeCacheKey(repoPath: string, wslDistro?: string): string { +// Why: gh intentionally executes on the native host even for connection-backed +// repos, but the answer still describes that connection's runtime (it is probed +// without the repository cwd). Keying it per connection stops a local repo and +// an SSH-hosted repo at the same path from adopting each other's auth state. +// Provider generation fences stale answers when reconnect replaces the runtime. +function runtimeCacheKey( + repoPath: string, + connectionId?: string | null, + wslDistro?: string +): string { + if (connectionId) { + return `connection:${connectionId}:${getSshGitProviderGeneration(connectionId)}` + } const resolvedDistro = wslDistro ?? parseWslPath(repoPath)?.distro return `local:${resolvedDistro?.toLowerCase() ?? 'host'}` } @@ -134,7 +145,7 @@ async function resolveAuthenticatedGitHubHost( localGitOptions: LocalGitExecOptions = {} ): Promise { const normalizedHost = normalizeGitHubHost(host)?.authority ?? host.trim().toLowerCase() - const cacheKey = `${runtimeCacheKey(repoPath, localGitOptions.wslDistro)}\0${normalizedHost}` + const cacheKey = `${runtimeCacheKey(repoPath, connectionId, localGitOptions.wslDistro)}\0${normalizedHost}` const now = Date.now() pruneHostAuthCache(now) const cached = hostAuthCache.get(cacheKey)