From 55a33c5b68771f2cb59d10fa427d1c0b02cd093c Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Sat, 30 May 2026 15:32:11 -0400 Subject: [PATCH] fix: expand backslash SSH home paths (#3861) * fix: tighten GitHub submit shortcut handling * fix: expand backslash ssh home paths --- .../ssh/ssh-agent-identity-filter.test.ts | 33 +++++++++- src/main/ssh/ssh-agent-identity-filter.ts | 12 +--- src/main/ssh/ssh-auth-resolution.ts | 22 ++----- src/main/ssh/ssh-config-parser.test.ts | 38 ++++++++--- src/main/ssh/ssh-config-parser.ts | 16 ++--- src/main/ssh/ssh-config-path-expansion.ts | 18 +++++ src/main/ssh/ssh-connection-utils.test.ts | 65 ++++++++++++++----- 7 files changed, 141 insertions(+), 63 deletions(-) create mode 100644 src/main/ssh/ssh-config-path-expansion.ts diff --git a/src/main/ssh/ssh-agent-identity-filter.test.ts b/src/main/ssh/ssh-agent-identity-filter.test.ts index 536ba3e7c8f..014d51e7b32 100644 --- a/src/main/ssh/ssh-agent-identity-filter.test.ts +++ b/src/main/ssh/ssh-agent-identity-filter.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import { join } from 'path' const mocks = vi.hoisted(() => ({ createAgent: vi.fn(), @@ -27,6 +28,12 @@ vi.mock('ssh2', () => { import { createIdentityFilteredAgent } from './ssh-agent-identity-filter' +const TEST_HOME = '/home/testuser' + +function testHomePath(...parts: string[]): string { + return join(TEST_HOME, ...parts) +} + type TestKey = { id: string equals: ReturnType @@ -68,7 +75,31 @@ describe('createIdentityFilteredAgent', () => { }) expect(identities).toEqual([allowedKey]) - expect(mocks.readFileSync).toHaveBeenCalledWith('/home/testuser/.ssh/work_key.pub') + expect(mocks.readFileSync).toHaveBeenCalledWith(testHomePath('.ssh', 'work_key.pub')) + }) + + it('expands Windows-style configured identity file paths before filtering', async () => { + const allowedKey = makeKey('allowed') + mocks.readFileSync.mockReturnValue('ssh-ed25519 AAAA allowed') + mocks.parseKey.mockReturnValue(allowedKey) + mocks.createAgent.mockReturnValue({ + getIdentities: vi.fn((callback) => callback(undefined, [allowedKey])), + sign: vi.fn() + }) + + const agent = createIdentityFilteredAgent('/tmp/agent.sock', ['~\\.ssh\\work_key']) + const identities = await new Promise((resolve, reject) => { + agent?.getIdentities((error, keys) => { + if (error) { + reject(error) + return + } + resolve(keys ?? []) + }) + }) + + expect(identities).toEqual([allowedKey]) + expect(mocks.readFileSync).toHaveBeenCalledWith(testHomePath('.ssh', 'work_key.pub')) }) it('filters nested public key entries returned by ssh2 agents', async () => { diff --git a/src/main/ssh/ssh-agent-identity-filter.ts b/src/main/ssh/ssh-agent-identity-filter.ts index 3e821188f9a..dbdfe477728 100644 --- a/src/main/ssh/ssh-agent-identity-filter.ts +++ b/src/main/ssh/ssh-agent-identity-filter.ts @@ -1,6 +1,4 @@ import { readFileSync } from 'fs' -import { homedir } from 'os' -import { join } from 'path' import { BaseAgent, createAgent, @@ -11,16 +9,10 @@ import { type SignCallback, type SigningRequestOptions } from 'ssh2' +import { resolveSshConfigHomePath } from './ssh-config-path-expansion' type AgentPublicKey = ParsedKey | Buffer | string | PublicKeyEntry -function resolveHomePath(filepath: string): string { - if (filepath.startsWith('~/') || filepath === '~') { - return join(homedir(), filepath.slice(1)) - } - return filepath -} - function comparablePublicKey(key: AgentPublicKey): ParsedKey | Buffer | string { if (typeof key === 'object' && 'pubKey' in key) { const pubKey = key.pubKey @@ -91,7 +83,7 @@ function parseIdentityKeyFile(filePath: string): ParsedKey | undefined { function readIdentityKeys(paths: string[]): ParsedKey[] { const keys: ParsedKey[] = [] for (const path of paths) { - const identityPath = resolveHomePath(path) + const identityPath = resolveSshConfigHomePath(path) const key = parseIdentityKeyFile(`${identityPath}.pub`) ?? parseIdentityKeyFile(identityPath) if (key) { keys.push(key) diff --git a/src/main/ssh/ssh-auth-resolution.ts b/src/main/ssh/ssh-auth-resolution.ts index 07f7f4cf54c..7be6ce405de 100644 --- a/src/main/ssh/ssh-auth-resolution.ts +++ b/src/main/ssh/ssh-auth-resolution.ts @@ -1,10 +1,9 @@ import { existsSync, readFileSync } from 'fs' -import { homedir } from 'os' -import { join } from 'path' import { utils, type BaseAgent, type ParsedKey } from 'ssh2' import type { SshTarget } from '../../shared/ssh-types' import type { SshResolvedConfig } from './ssh-config-parser' import { createIdentityFilteredAgent } from './ssh-agent-identity-filter' +import { resolveSshConfigHomePath } from './ssh-config-path-expansion' // Why: ssh2 only tries keys that are explicitly provided. Users with keys in // standard locations (e.g. ~/.ssh/id_ed25519) but no SSH agent running would @@ -14,15 +13,15 @@ const DEFAULT_KEY_NAMES = ['id_ed25519', 'id_rsa', 'id_ecdsa', 'id_dsa', 'id_xms const DEFAULT_KEY_PATHS = DEFAULT_KEY_NAMES.map((name) => `~/.ssh/${name}`) const WINDOWS_OPENSSH_AGENT_PIPE = '\\\\.\\pipe\\openssh-ssh-agent' -// Why: parseSshGOutput expands ~ to homedir(), so resolved identityFile -// paths won't match the ~/... form in DEFAULT_KEY_PATHS. -const EXPANDED_DEFAULT_KEY_PATHS = DEFAULT_KEY_NAMES.map((name) => join(homedir(), '.ssh', name)) +// Why: resolved IdentityFile paths are expanded before auth resolution, so they +// won't match the ~/... form in DEFAULT_KEY_PATHS. +const EXPANDED_DEFAULT_KEY_PATHS = DEFAULT_KEY_PATHS.map(resolveSshConfigHomePath) export type PrivateKeyFile = { path: string; contents: Buffer } export function findDefaultKeyFile(): PrivateKeyFile | undefined { for (const keyPath of DEFAULT_KEY_PATHS) { - const resolved = keyPath.replace(/^~/, homedir()) + const resolved = resolveSshConfigHomePath(keyPath) try { if (!existsSync(resolved)) { continue @@ -36,13 +35,6 @@ export function findDefaultKeyFile(): PrivateKeyFile | undefined { return undefined } -function resolveHomePath(filepath: string): string { - if (filepath.startsWith('~/') || filepath === '~') { - return join(homedir(), filepath.slice(1)) - } - return filepath -} - function expandIdentityAgentEnv(value: string): string | undefined { if (value === 'SSH_AUTH_SOCK') { return process.env.SSH_AUTH_SOCK || undefined @@ -83,7 +75,7 @@ export function resolveAgentSocket( if (!trimmed || trimmed.toLowerCase() === 'none') { return undefined } - return expandIdentityAgentEnv(resolveHomePath(trimmed)) + return expandIdentityAgentEnv(resolveSshConfigHomePath(trimmed)) } return resolveDefaultAgentSocket() } @@ -103,7 +95,7 @@ function resolveExplicitPrivateKeyPath( function readPrivateKey(keyPath: string): PrivateKeyFile | undefined { try { - const resolvedPath = resolveHomePath(keyPath) + const resolvedPath = resolveSshConfigHomePath(keyPath) return { path: keyPath, contents: readFileSync(resolvedPath) } } catch { return undefined diff --git a/src/main/ssh/ssh-config-parser.test.ts b/src/main/ssh/ssh-config-parser.test.ts index 541742df8c0..2d5f8e3c2af 100644 --- a/src/main/ssh/ssh-config-parser.test.ts +++ b/src/main/ssh/ssh-config-parser.test.ts @@ -1,5 +1,6 @@ /* eslint-disable max-lines -- Why: SSH config parsing fixtures cover OpenSSH file parsing and ssh -G output together so import and connection resolution stay aligned. */ import { describe, expect, it, vi } from 'vitest' +import { join } from 'path' import { parseSshConfig, sshConfigHostsToTargets, parseSshGOutput } from './ssh-config-parser' vi.mock('os', () => ({ @@ -7,6 +8,11 @@ vi.mock('os', () => ({ })) const LARGE_HOST_ALIAS_COUNT = 150_000 +const TEST_HOME = '/home/testuser' + +function testHomePath(...parts: string[]): string { + return join(TEST_HOME, ...parts) +} function buildHostAliases(count: number): string { const aliases: string[] = [] @@ -85,7 +91,17 @@ Host myserver IdentityFile ~/.ssh/id_ed25519 ` const hosts = parseSshConfig(config) - expect(hosts[0].identityFile).toBe('/home/testuser/.ssh/id_ed25519') + expect(hosts[0].identityFile).toBe(testHomePath('.ssh', 'id_ed25519')) + }) + + it('parses Windows-style IdentityFile with ~ expansion', () => { + const config = ` +Host myserver + HostName example.com + IdentityFile ~\\.ssh\\id_ed25519 +` + const hosts = parseSshConfig(config) + expect(hosts[0].identityFile).toBe(testHomePath('.ssh', 'id_ed25519')) }) it('parses IdentityAgent with ~ expansion', () => { @@ -95,7 +111,7 @@ Host myserver IdentityAgent ~/.1password/agent.sock ` const hosts = parseSshConfig(config) - expect(hosts[0].identityAgent).toBe('/home/testuser/.1password/agent.sock') + expect(hosts[0].identityAgent).toBe(testHomePath('.1password', 'agent.sock')) }) it('parses IdentitiesOnly', () => { @@ -217,12 +233,12 @@ Host staging stage expect(hosts).toEqual([ { host: 'staging', - identityAgent: '/home/testuser/.1password/agent.sock', + identityAgent: testHomePath('.1password', 'agent.sock'), identitiesOnly: true }, { host: 'stage', - identityAgent: '/home/testuser/.1password/agent.sock', + identityAgent: testHomePath('.1password', 'agent.sock'), identitiesOnly: true } ]) @@ -341,8 +357,8 @@ describe('parseSshGOutput', () => { const result = parseSshGOutput(output) expect(result.identityFile).toEqual([ - '/home/testuser/.ssh/id_ed25519', - '/home/testuser/.ssh/id_rsa' + testHomePath('.ssh', 'id_ed25519'), + testHomePath('.ssh', 'id_rsa') ]) }) @@ -417,13 +433,19 @@ describe('parseSshGOutput', () => { it('handles ~ expansion in identity file paths', () => { const output = 'hostname example.com\nidentityfile ~/custom_key\nport 22' const result = parseSshGOutput(output) - expect(result.identityFile).toEqual(['/home/testuser/custom_key']) + expect(result.identityFile).toEqual([testHomePath('custom_key')]) + }) + + it('handles Windows-style ~ expansion in identity file paths', () => { + const output = 'hostname example.com\nidentityfile ~\\.ssh\\custom_key\nport 22' + const result = parseSshGOutput(output) + expect(result.identityFile).toEqual([testHomePath('.ssh', 'custom_key')]) }) it('parses identityagent with ~ expansion', () => { const output = 'hostname example.com\nidentityagent ~/.1password/agent.sock\nport 22' const result = parseSshGOutput(output) - expect(result.identityAgent).toBe('/home/testuser/.1password/agent.sock') + expect(result.identityAgent).toBe(testHomePath('.1password', 'agent.sock')) }) it('preserves identityagent none so auth can disable agent fallback', () => { diff --git a/src/main/ssh/ssh-config-parser.ts b/src/main/ssh/ssh-config-parser.ts index c8c02370d35..4f59d508798 100644 --- a/src/main/ssh/ssh-config-parser.ts +++ b/src/main/ssh/ssh-config-parser.ts @@ -4,6 +4,7 @@ import { join } from 'path' import { homedir } from 'os' import type { SshTarget } from '../../shared/ssh-types' import { expandSshConfigIncludes } from './ssh-config-include-expander' +import { resolveSshConfigHomePath } from './ssh-config-path-expansion' export type SshConfigHost = { host: string @@ -90,12 +91,12 @@ export function parseSshConfig(content: string): SshConfigHost[] { break case 'identityfile': for (const host of current) { - host.identityFile = resolveHomePath(value) + host.identityFile = resolveSshConfigHomePath(value) } break case 'identityagent': for (const host of current) { - host.identityAgent = resolveHomePath(value) + host.identityAgent = resolveSshConfigHomePath(value) } break case 'identitiesonly': @@ -181,13 +182,6 @@ function splitHostPatterns(input: string): string[] { return patterns } -function resolveHomePath(filepath: string): string { - if (filepath.startsWith('~/') || filepath === '~') { - return join(homedir(), filepath.slice(1)) - } - return filepath -} - /** Read and parse the user's ~/.ssh/config file. Returns empty array if not found. */ export function loadUserSshConfig(): SshConfigHost[] { const configPath = join(homedir(), '.ssh', 'config') @@ -311,7 +305,7 @@ export function parseSshGOutput(stdout: string): SshResolvedConfig { const key = line.substring(0, spaceIdx).toLowerCase() const value = line.substring(spaceIdx + 1).trim() if (key === 'identityfile') { - identityFiles.push(resolveHomePath(value)) + identityFiles.push(resolveSshConfigHomePath(value)) } else { map.set(key, value) } @@ -324,7 +318,7 @@ export function parseSshGOutput(stdout: string): SshResolvedConfig { const rawJump = map.get('proxyjump') const proxyJump = rawJump && rawJump !== 'none' ? rawJump : undefined const rawIdentityAgent = map.get('identityagent') - const identityAgent = rawIdentityAgent ? resolveHomePath(rawIdentityAgent) : undefined + const identityAgent = rawIdentityAgent ? resolveSshConfigHomePath(rawIdentityAgent) : undefined return { hostname: map.get('hostname') ?? '', diff --git a/src/main/ssh/ssh-config-path-expansion.ts b/src/main/ssh/ssh-config-path-expansion.ts new file mode 100644 index 00000000000..71ec58fc19b --- /dev/null +++ b/src/main/ssh/ssh-config-path-expansion.ts @@ -0,0 +1,18 @@ +import { homedir } from 'os' +import { join } from 'path' + +export function resolveSshConfigHomePath(filepath: string): string { + if (filepath === '~') { + return homedir() + } + if (filepath.startsWith('~/') || filepath.startsWith('~\\')) { + return join( + homedir(), + ...filepath + .slice(2) + .split(/[\\/]+/) + .filter(Boolean) + ) + } + return filepath +} diff --git a/src/main/ssh/ssh-connection-utils.test.ts b/src/main/ssh/ssh-connection-utils.test.ts index f075c9532ff..de1a42a4b8a 100644 --- a/src/main/ssh/ssh-connection-utils.test.ts +++ b/src/main/ssh/ssh-connection-utils.test.ts @@ -1,5 +1,6 @@ /* eslint-disable max-lines -- Why: SSH connection utility tests share mocked filesystem and environment setup across auth, proxy, and retry helpers. */ import { afterEach, describe, expect, it, vi, beforeEach } from 'vitest' +import { join } from 'path' import { BaseAgent, utils, type ParsedKey } from 'ssh2' vi.mock('os', () => ({ @@ -8,6 +9,11 @@ vi.mock('os', () => ({ const mockExistsSync = vi.fn().mockReturnValue(false) const mockReadFileSync = vi.fn() +const TEST_HOME = '/home/testuser' + +function testHomePath(...parts: string[]): string { + return join(TEST_HOME, ...parts) +} vi.mock('fs', () => ({ existsSync: (...args: unknown[]) => mockExistsSync(...args), @@ -205,7 +211,7 @@ describe('findDefaultKeyFile', () => { it('returns the first existing key file', () => { mockExistsSync.mockImplementation((path: unknown) => { - return path === '/home/testuser/.ssh/id_ed25519' + return path === testHomePath('.ssh', 'id_ed25519') }) mockReadFileSync.mockReturnValue(Buffer.from('key-contents')) @@ -225,20 +231,20 @@ describe('findDefaultKeyFile', () => { findDefaultKeyFile() expect(checkedPaths).toEqual([ - '/home/testuser/.ssh/id_ed25519', - '/home/testuser/.ssh/id_rsa', - '/home/testuser/.ssh/id_ecdsa', - '/home/testuser/.ssh/id_dsa', - '/home/testuser/.ssh/id_xmss' + testHomePath('.ssh', 'id_ed25519'), + testHomePath('.ssh', 'id_rsa'), + testHomePath('.ssh', 'id_ecdsa'), + testHomePath('.ssh', 'id_dsa'), + testHomePath('.ssh', 'id_xmss') ]) }) it('skips unreadable key files and tries next', () => { mockExistsSync.mockImplementation((path: unknown) => { - return path === '/home/testuser/.ssh/id_ed25519' || path === '/home/testuser/.ssh/id_rsa' + return path === testHomePath('.ssh', 'id_ed25519') || path === testHomePath('.ssh', 'id_rsa') }) mockReadFileSync.mockImplementation((path: unknown) => { - if (String(path) === '/home/testuser/.ssh/id_ed25519') { + if (String(path) === testHomePath('.ssh', 'id_ed25519')) { throw new Error('permission denied') } return Buffer.from('rsa-key') @@ -337,9 +343,9 @@ describe('buildConnectConfig', () => { it('prefers ssh -G resolved IdentityAgent for config-host targets', () => { const config = buildConnectConfig( makeTarget({ configHost: 'work', identityAgent: '%d/.1password/agent.sock' }), - makeResolved({ identityAgent: '/home/testuser/.1password/agent.sock' }) + makeResolved({ identityAgent: testHomePath('.1password', 'agent.sock') }) ) - expect(config.agent).toBe('/home/testuser/.1password/agent.sock') + expect(config.agent).toBe(testHomePath('.1password', 'agent.sock')) }) it('allows IdentityAgent none to disable agent auth', () => { @@ -419,11 +425,29 @@ describe('buildConnectConfig', () => { }) it('uses keyFile auth when target.identityFile is set and no agent is available', () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('linux') delete process.env.SSH_AUTH_SOCK mockReadFileSync.mockReturnValue(Buffer.from('key')) - const config = buildConnectConfig(makeTarget({ identityFile: '/home/user/.ssh/custom' }), null) + try { + const config = buildConnectConfig( + makeTarget({ identityFile: '/home/user/.ssh/custom' }), + null + ) + expect(config.privateKey).toEqual(Buffer.from('key')) + expect(config.agent).toBeUndefined() + } finally { + platformSpy.mockRestore() + } + }) + + it('expands Windows-style target.identityFile before reading private key', () => { + mockReadFileSync.mockReturnValue(Buffer.from('key')) + const config = buildConnectConfig(makeTarget({ identityFile: '~\\.ssh\\custom' }), null, { + includeAgent: false, + includePrivateKey: true + }) expect(config.privateKey).toEqual(Buffer.from('key')) - expect(config.agent).toBeUndefined() + expect(mockReadFileSync).toHaveBeenCalledWith(testHomePath('.ssh', 'custom')) }) it('includes unencrypted resolved identityFile auth when an agent is available', () => { @@ -442,7 +466,7 @@ describe('buildConnectConfig', () => { it('uses agent auth without probing when resolved identityFile is a default path (expanded)', () => { const config = buildConnectConfig( makeTarget(), - makeResolved({ identityFile: ['/home/testuser/.ssh/id_ed25519'] }) + makeResolved({ identityFile: [testHomePath('.ssh', 'id_ed25519')] }) ) expect(config.agent).toBe('/tmp/agent.sock') expect(config.privateKey).toBeUndefined() @@ -451,7 +475,7 @@ describe('buildConnectConfig', () => { it('does not probe default key files before agent auth', () => { mockExistsSync.mockImplementation( - (p: unknown) => String(p) === '/home/testuser/.ssh/id_ed25519' + (p: unknown) => String(p) === testHomePath('.ssh', 'id_ed25519') ) const config = buildConnectConfig(makeTarget(), null) expect(config.agent).toBe('/tmp/agent.sock') @@ -460,14 +484,19 @@ describe('buildConnectConfig', () => { }) it('provides fallback key when no agent is available', () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('linux') delete process.env.SSH_AUTH_SOCK mockExistsSync.mockImplementation( - (p: unknown) => String(p) === '/home/testuser/.ssh/id_ed25519' + (p: unknown) => String(p) === testHomePath('.ssh', 'id_ed25519') ) mockReadFileSync.mockReturnValue(Buffer.from('fallback')) - const config = buildConnectConfig(makeTarget(), null) - expect(config.agent).toBeUndefined() - expect(config.privateKey).toEqual(Buffer.from('fallback')) + try { + const config = buildConnectConfig(makeTarget(), null) + expect(config.agent).toBeUndefined() + expect(config.privateKey).toEqual(Buffer.from('fallback')) + } finally { + platformSpy.mockRestore() + } }) it('can force private key inclusion for the post-agent fallback path', () => {