diff --git a/src/main/ssh/ssh-config-host-picker.test.ts b/src/main/ssh/ssh-config-host-picker.test.ts index 783eef679ca..3ac84902dbf 100644 --- a/src/main/ssh/ssh-config-host-picker.test.ts +++ b/src/main/ssh/ssh-config-host-picker.test.ts @@ -57,6 +57,58 @@ describe('SSH config host picker search', () => { expect(last.hasMore).toBe(false) }) + it('flags a config host whose machine is already registered under another label', () => { + // Why: the same box gets added twice — once manually by IP, once from the config + // picker — and alias comparison cannot see that because the names differ. + const result = searchSshConfigHosts( + [{ host: 'aimp', hostname: '172.16.203.131', user: 'chanmuzi' }], + [ + { + configHost: '172.16.203.131', + label: 'AI Box', + host: '172.16.203.131', + port: 22, + username: 'chanmuzi' + } + ] + ) + + expect(result.hosts[0]).toMatchObject({ alias: 'aimp', alreadyInOrca: true }) + expect(result.newHostCount).toBe(0) + }) + + it('leaves a different machine unflagged', () => { + const result = searchSshConfigHosts( + [{ host: 'aimp', hostname: '172.16.203.131', user: 'chanmuzi' }], + [ + { + configHost: '10.0.0.9', + label: 'Other Box', + host: '10.0.0.9', + port: 22, + username: 'chanmuzi' + } + ] + ) + + expect(result.hosts[0]).toMatchObject({ alias: 'aimp', alreadyInOrca: false }) + expect(result.newHostCount).toBe(1) + }) + + it('does not collide config hosts that carry no HostName', () => { + // Why: without HostName the candidate host degrades to the alias itself, so the + // endpoint check must not start matching unrelated aliases against each other. + const result = searchSshConfigHosts( + [{ host: 'alpha' }, { host: 'beta' }], + [{ configHost: 'gamma', label: 'Gamma', host: 'gamma', port: 22, username: '' }] + ) + + expect(result.hosts.map((host) => [host.alias, host.alreadyInOrca])).toEqual([ + ['alpha', false], + ['beta', false] + ]) + }) + it('counts de-duplicated and existing aliases while matching effective display fields', () => { const result = searchSshConfigHosts( [ diff --git a/src/main/ssh/ssh-config-host-picker.ts b/src/main/ssh/ssh-config-host-picker.ts index d1870ad1b69..3fcc98859c1 100644 --- a/src/main/ssh/ssh-config-host-picker.ts +++ b/src/main/ssh/ssh-config-host-picker.ts @@ -5,7 +5,7 @@ import type { SshTarget } from '../../shared/ssh-types' import { SSH_CONFIG_HOST_RESULT_LIMIT } from '../../shared/ssh-types' -import { normalizeSshConfigAlias } from '../../shared/ssh-config-alias' +import { normalizeSshConfigAlias, sshEndpointIdentity } from '../../shared/ssh-config-alias' import { loadUserSshConfig, type SshConfigHost } from './ssh-config-parser' import { resolveWithSshG, type SshResolvedConfig } from './ssh-g-config-resolution' @@ -27,7 +27,10 @@ function getUserSshConfigHosts(refresh: boolean): SshConfigHost[] { export function searchSshConfigHosts( hosts: SshConfigHost[], - existingTargets: readonly Pick[], + // Why partial: callers that only know the names still type-check, and an entry with + // no host yields an empty identity rather than a false match. + existingTargets: readonly (Pick & + Partial>)[], query = '', suppressedAliases: readonly string[] = [] ): SshConfigHostListResult { @@ -36,6 +39,9 @@ export function searchSshConfigHosts( [target.configHost, target.label].map(normalizeSshConfigAlias).filter(Boolean) ) ) + // Why: the same machine can be registered twice under different names — once by IP, + // once from this picker — and comparing names cannot see that. + const existingEndpoints = new Set(existingTargets.map(sshEndpointIdentity).filter(Boolean)) const normalizedQuery = query.trim().toLowerCase() const suppressedAliasSet = new Set(suppressedAliases.map(normalizeSshConfigAlias)) const summaries: SshConfigHostSummary[] = [] @@ -50,7 +56,13 @@ export function searchSshConfigHosts( continue } seenAliases.add(normalizedAlias) - const alreadyInOrca = existingAliases.has(normalizedAlias) + // Why `entry.hostname` only: without HostName the candidate host degrades to the + // alias itself, and matching on that would just re-run the alias check. + const endpoint = entry.hostname + ? sshEndpointIdentity({ host: entry.hostname, port: entry.port, username: entry.user }) + : '' + const alreadyInOrca = + existingAliases.has(normalizedAlias) || (endpoint !== '' && existingEndpoints.has(endpoint)) // Why: tombstones only block passive bulk import — the picker still lists the // Host so deleting one Orca target never looks like "~/.ssh/config is empty". const previouslyRemoved = !alreadyInOrca && suppressedAliasSet.has(normalizedAlias) @@ -76,7 +88,10 @@ export function searchSshConfigHosts( } export function listUserSshConfigHostSummaries( - existingTargets: readonly Pick[], + // Why partial: callers that only know the names still type-check, and an entry with + // no host yields an empty identity rather than a false match. + existingTargets: readonly (Pick & + Partial>)[], query?: string, suppressedAliases?: readonly string[], options?: { refresh?: boolean } diff --git a/src/shared/ssh-config-alias.test.ts b/src/shared/ssh-config-alias.test.ts new file mode 100644 index 00000000000..1e154b38bdb --- /dev/null +++ b/src/shared/ssh-config-alias.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from 'vitest' +import { sshEndpointIdentity } from './ssh-config-alias' + +describe('sshEndpointIdentity', () => { + it('treats the same machine reached under different names as one endpoint', () => { + const manual = sshEndpointIdentity({ + host: '172.16.203.131', + port: 22, + username: 'chanmuzi' + }) + const fromConfig = sshEndpointIdentity({ + host: '172.16.203.131', + port: 22, + username: 'chanmuzi' + }) + + expect(manual).toBe(fromConfig) + }) + + it('defaults a missing port to 22 so an explicit 22 matches an omitted one', () => { + expect(sshEndpointIdentity({ host: 'box', username: 'me' })).toBe( + sshEndpointIdentity({ host: 'box', port: 22, username: 'me' }) + ) + }) + + it('separates host, port and user so no concatenation can alias them', () => { + // Why: 'a' + '1:2' and 'a:1' + '2' must not collapse into the same key. + expect(sshEndpointIdentity({ host: 'a', port: 12, username: '' })).not.toBe( + sshEndpointIdentity({ host: 'a', port: 1, username: '2' }) + ) + }) + + it('folds host and user case but keeps a different user distinct', () => { + expect(sshEndpointIdentity({ host: 'BOX', port: 22, username: 'Me' })).toBe( + sshEndpointIdentity({ host: 'box', port: 22, username: 'me' }) + ) + expect(sshEndpointIdentity({ host: 'box', port: 22, username: 'me' })).not.toBe( + sshEndpointIdentity({ host: 'box', port: 22, username: 'other' }) + ) + }) + + it('is empty for an unknown host so unresolved entries never collide', () => { + expect(sshEndpointIdentity({ host: '', port: 22, username: 'me' })).toBe('') + expect(sshEndpointIdentity({ host: ' ', port: 22, username: 'me' })).toBe('') + expect(sshEndpointIdentity({ port: 22, username: 'me' })).toBe('') + }) + + it('ignores a non-finite port rather than keying on NaN', () => { + expect(sshEndpointIdentity({ host: 'box', port: Number.NaN, username: 'me' })).toBe( + sshEndpointIdentity({ host: 'box', port: 22, username: 'me' }) + ) + }) +}) diff --git a/src/shared/ssh-config-alias.ts b/src/shared/ssh-config-alias.ts index ea93dd75de4..b5800c08cab 100644 --- a/src/shared/ssh-config-alias.ts +++ b/src/shared/ssh-config-alias.ts @@ -6,3 +6,18 @@ export function normalizeSshConfigAlias(alias: string | null | undefined): string { return alias ? alias.trim().toLowerCase() : '' } + +/** The machine a target actually reaches: hostname, port and user, normalized. + * Empty when the host is unknown, so an unresolved alias never collides. */ +export function sshEndpointIdentity(target: { + host?: string | null + port?: number | null + username?: string | null +}): string { + const host = target.host?.trim().toLowerCase() ?? '' + if (!host) { + return '' + } + const port = typeof target.port === 'number' && Number.isFinite(target.port) ? target.port : 22 + return `${host}\u0000${port}\u0000${target.username?.trim().toLowerCase() ?? ''}` +}