From 28e2ce929ffdd5fc358a9dbc35001ec40dbcfe47 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 30 May 2026 13:10:39 -0700 Subject: [PATCH] Improve SSH config target handling (#3899) --- docs/ssh-config-target-compatibility.md | 61 ++++++ src/main/ssh/ssh-connection-utils.test.ts | 47 +++++ src/main/ssh/ssh-connection-utils.ts | 32 +++- .../src/components/settings/SshPane.tsx | 31 ++-- .../src/components/settings/SshTargetCard.tsx | 5 +- .../src/components/settings/SshTargetForm.tsx | 35 +--- .../settings/ssh-target-draft.test.ts | 86 +++++++++ .../components/settings/ssh-target-draft.ts | 173 ++++++++++++++++++ 8 files changed, 421 insertions(+), 49 deletions(-) create mode 100644 docs/ssh-config-target-compatibility.md create mode 100644 src/renderer/src/components/settings/ssh-target-draft.test.ts create mode 100644 src/renderer/src/components/settings/ssh-target-draft.ts diff --git a/docs/ssh-config-target-compatibility.md b/docs/ssh-config-target-compatibility.md new file mode 100644 index 000000000..07817dc41 --- /dev/null +++ b/docs/ssh-config-target-compatibility.md @@ -0,0 +1,61 @@ +# SSH Config Target Compatibility + +## Problem + +- `src/main/ssh/ssh-connection-utils.ts:108` resolves OpenSSH config with `ssh -G`, but then prefers the persisted `target.host` over `resolved.hostname`. +- `src/main/ssh/ssh-connection-utils.ts:109` treats persisted port `22` as an explicit override, so a config-host target can ignore a resolved non-default `Port`. +- `src/main/ssh/ssh-config-parser.ts:210` imports config aliases with `host` set to the alias when the concrete `Host` block lacks an inline `HostName`; later `ssh -G` may know the real host, but the connection path ignores it. +- `src/renderer/src/components/settings/SshPane.tsx:63` requires both host and username, even though OpenSSH config aliases can resolve the user and users commonly paste `user@host:port` targets. +- `src/main/ssh/ssh-config-parser.ts:330` parses `ForwardAgent`, but `src/main/ssh/ssh-connection-utils.ts:112` did not pass it into ssh2, so remote git commands that rely on the local agent could fail. + +## Goal + +Make Orca behave like a mature SSH client for common config-host flows: aliases imported from `~/.ssh/config`, aliases with inherited `HostName`/`Port`, and pasted SSH targets should connect without users re-entering information that OpenSSH can resolve. + +## Non-goals + +- Do not add persistent secret storage for SSH passwords or key passphrases. +- Do not redesign the whole SSH settings page. +- Do not change relay deployment or remote PTY lease semantics. +- Do not add a known-host trust UI in this patch. + +## Design + +1. Preserve explicit target overrides while letting config aliases use resolved values. + - In `buildConnectConfig`, prefer `resolved.hostname` only when the persisted host is blank, the same as `configHost`, or the same as the label. + - Prefer `resolved.port` when the target is a config-host target still on the default `22`; keep non-default target ports as explicit overrides. + - Continue using target username first, then resolved user. + +2. Honor resolved agent forwarding where ssh2 can support it. + - Set `agentForward` only when resolved config requested forwarding and an agent is actually configured. + - Leave system-SSH transport unchanged because it already delegates to OpenSSH config. + +3. Normalize settings form drafts before save. + - Accept `ssh://user@host:port`, `user@host:port`, and plain aliases in the Host field. + - Auto-fill username and port from pasted inputs only when the dedicated fields are still empty/default. + - Allow username to be omitted; `ssh -G` can provide it during connect. + +4. Keep UI changes small. + - Rename copy only where needed to avoid implying username is mandatory. + - Render username-less targets without a leading `@`. + - Do not introduce new colors, typography, or layout patterns. + +5. Cover behavior with focused tests. + - Add connection-config tests for config-host resolved hostname/port precedence. + - Add connection-config tests for `ForwardAgent yes`. + - Add renderer utility tests for pasted SSH target normalization. + +## Edge Cases + +- Explicit non-default port in Orca still wins over `ssh -G`. +- Empty or unparsable host input remains invalid. +- IPv6 bracket syntax is accepted for `ssh://` URLs and preserved conservatively for scp-like inputs. +- Plain config aliases remain valid even without a username. + +## Rollout + +1. Add the renderer draft-normalization helper and tests. +2. Wire the SSH settings form save path and labels to the helper. +3. Update `buildConnectConfig` precedence/agent-forwarding and tests. +4. Clean up username-less target display. +5. Run focused tests and typecheck/lint where feasible. diff --git a/src/main/ssh/ssh-connection-utils.test.ts b/src/main/ssh/ssh-connection-utils.test.ts index 4dbc9ec6a..0af7103b1 100644 --- a/src/main/ssh/ssh-connection-utils.test.ts +++ b/src/main/ssh/ssh-connection-utils.test.ts @@ -343,6 +343,33 @@ describe('buildConnectConfig', () => { expect(config.username).toBe('admin') }) + it('uses ssh -G HostName when a config-host target still points at its alias', () => { + const config = buildConnectConfig( + makeTarget({ label: 'workbox', configHost: 'workbox', host: 'workbox' }), + makeResolved({ hostname: 'workbox.internal' }) + ) + + expect(config.host).toBe('workbox.internal') + }) + + it('uses ssh -G Port when a config-host target still has the default port', () => { + const config = buildConnectConfig( + makeTarget({ configHost: 'workbox', host: 'workbox', port: 22 }), + makeResolved({ port: 2202 }) + ) + + expect(config.port).toBe(2202) + }) + + it('keeps explicit non-default target ports ahead of ssh -G Port', () => { + const config = buildConnectConfig( + makeTarget({ configHost: 'workbox', host: 'workbox', port: 2022 }), + makeResolved({ port: 2202 }) + ) + + expect(config.port).toBe(2022) + }) + it('sets readyTimeout to CONNECT_TIMEOUT_MS', () => { const config = buildConnectConfig(makeTarget(), null) expect(config.readyTimeout).toBe(30_000) @@ -358,6 +385,26 @@ describe('buildConnectConfig', () => { expect(config.agent).toBe('/tmp/agent.sock') }) + it('enables agent forwarding when OpenSSH config requests it and an agent is available', () => { + const config = buildConnectConfig(makeTarget(), makeResolved({ forwardAgent: true })) + + expect(config.agent).toBe('/tmp/agent.sock') + expect(config.agentForward).toBe(true) + }) + + it('does not enable agent forwarding without a usable agent', () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('linux') + delete process.env.SSH_AUTH_SOCK + + try { + const config = buildConnectConfig(makeTarget(), makeResolved({ forwardAgent: true })) + expect(config.agent).toBeUndefined() + expect(config.agentForward).toBeUndefined() + } finally { + platformSpy.mockRestore() + } + }) + it('uses configured IdentityAgent before SSH_AUTH_SOCK', () => { const config = buildConnectConfig( makeTarget(), diff --git a/src/main/ssh/ssh-connection-utils.ts b/src/main/ssh/ssh-connection-utils.ts index 35995edd4..0e22dede8 100644 --- a/src/main/ssh/ssh-connection-utils.ts +++ b/src/main/ssh/ssh-connection-utils.ts @@ -105,8 +105,8 @@ export function buildConnectConfig( resolved: SshResolvedConfig | null, options: BuildConnectConfigOptions = {} ): ConnectConfig { - const effectiveHost = target.host || resolved?.hostname || target.label - const effectivePort = target.port || resolved?.port || 22 + const effectiveHost = resolveEffectiveHost(target, resolved) + const effectivePort = resolveEffectivePort(target, resolved) const effectiveUser = target.username || resolved?.user || '' const config: Record = { @@ -125,6 +125,10 @@ export function buildConnectConfig( config.agent = agent } + if (agent && resolved?.forwardAgent) { + config.agentForward = true + } + const key = (options.includePrivateKey ?? !agent) ? resolvePrivateKey(target, resolved) @@ -136,6 +140,30 @@ export function buildConnectConfig( return config as ConnectConfig } +function resolveEffectiveHost(target: SshTarget, resolved: SshResolvedConfig | null): string { + if (shouldUseResolvedEndpoint(target, resolved)) { + return resolved!.hostname + } + return target.host || resolved?.hostname || target.label +} + +function resolveEffectivePort(target: SshTarget, resolved: SshResolvedConfig | null): number { + // Why: imported config aliases store 22 as the schema default even when an + // included/wildcard OpenSSH rule later resolves a different effective Port. + if (target.configHost && target.port === 22 && resolved?.port) { + return resolved.port + } + return target.port || resolved?.port || 22 +} + +function shouldUseResolvedEndpoint(target: SshTarget, resolved: SshResolvedConfig | null): boolean { + if (!target.configHost || !resolved?.hostname) { + return false + } + const host = target.host.trim() + return host === '' || host === target.configHost || host === target.label +} + // Why: ProxyJump and jumpHost are syntactic sugar for ProxyCommand. // OpenSSH internally converts `ProxyJump bastion` to // `ProxyCommand ssh -W %h:%p bastion`. We do the same so that ssh2 diff --git a/src/renderer/src/components/settings/SshPane.tsx b/src/renderer/src/components/settings/SshPane.tsx index fcd372481..f016e506b 100644 --- a/src/renderer/src/components/settings/SshPane.tsx +++ b/src/renderer/src/components/settings/SshPane.tsx @@ -4,7 +4,6 @@ import { Plus, Upload } from 'lucide-react' import { DEFAULT_SSH_RELAY_GRACE_PERIOD_SECONDS, MAX_SSH_RELAY_GRACE_PERIOD_SECONDS, - MIN_SSH_RELAY_GRACE_PERIOD_SECONDS, type SshTarget } from '../../../../shared/ssh-types' import { SSH_TERMINATE_RECONNECT_REQUIRED } from '../../../../shared/constants' @@ -15,6 +14,11 @@ import { removeSshTargetWithBestEffortCleanup } from './ssh-target-remove' import { SshTargetCard } from './SshTargetCard' import { SshTargetDestructiveActions } from './SshTargetDestructiveActions' import { SshTargetForm, EMPTY_FORM, type EditingTarget } from './SshTargetForm' +import { + getSshTargetDraftConnectionFields, + isRelayGracePeriodValid, + parseRelayGracePeriodSeconds +} from './ssh-target-draft' export { SSH_PANE_SEARCH_ENTRIES } from './ssh-search' type SshPaneProps = Record @@ -60,26 +64,19 @@ export function SshPane(_props: SshPaneProps): React.JSX.Element { }, [loadTargets]) const handleSave = async (): Promise => { - if (!form.host.trim() || !form.username.trim()) { - toast.error('Host and username are required') + const { host, configHost, username, port } = getSshTargetDraftConnectionFields(form) + if (!host) { + toast.error('Host or SSH config alias is required') return } - const port = parseInt(form.port, 10) if (isNaN(port) || port < 1 || port > 65535) { toast.error('Port must be between 1 and 65535') return } - const graceSeconds = form.relayKeepAliveUntilReset - ? 0 - : parseInt(form.relayGracePeriodSeconds, 10) - if ( - !form.relayKeepAliveUntilReset && - (isNaN(graceSeconds) || - graceSeconds < MIN_SSH_RELAY_GRACE_PERIOD_SECONDS || - graceSeconds > MAX_SSH_RELAY_GRACE_PERIOD_SECONDS) - ) { + const graceSeconds = parseRelayGracePeriodSeconds(form) + if (!isRelayGracePeriodValid(form, graceSeconds)) { toast.error( `Relay grace period must be between 60 and ${MAX_SSH_RELAY_GRACE_PERIOD_SECONDS} seconds, or choose keep alive until reset` ) @@ -87,11 +84,11 @@ export function SshPane(_props: SshPaneProps): React.JSX.Element { } const target = { - label: form.label.trim() || `${form.username}@${form.host}`, - configHost: form.configHost.trim() || form.host.trim(), - host: form.host.trim(), + label: form.label.trim() || (username ? `${username}@${host}` : configHost), + configHost, + host, port, - username: form.username.trim(), + username, relayGracePeriodSeconds: graceSeconds, ...(form.identityFile.trim() ? { identityFile: form.identityFile.trim() } : {}), ...(form.proxyCommand.trim() ? { proxyCommand: form.proxyCommand.trim() } : {}), diff --git a/src/renderer/src/components/settings/SshTargetCard.tsx b/src/renderer/src/components/settings/SshTargetCard.tsx index d794c64b9..821e807e5 100644 --- a/src/renderer/src/components/settings/SshTargetCard.tsx +++ b/src/renderer/src/components/settings/SshTargetCard.tsx @@ -86,6 +86,9 @@ export function SshTargetCard({ const resetInFlight = actionInFlight === 'reset' || busyAction === 'reset' const removeInFlight = busyAction === 'remove' const mountedRef = useRef(true) + const endpoint = target.username + ? `${target.username}@${target.host}:${target.port}` + : `${target.host}:${target.port}` const handleCardRef = useCallback((node: HTMLDivElement | null): void => { // Why: SSH target actions can resolve after the card is removed; the root @@ -238,7 +241,7 @@ export function SshTargetCard({ {STATUS_LABELS[status]}

- {target.username}@{target.host}:{target.port} + {endpoint} {target.identityFile ? ` \u2022 ${target.identityFile}` : ''}

{state?.error ? ( diff --git a/src/renderer/src/components/settings/SshTargetForm.tsx b/src/renderer/src/components/settings/SshTargetForm.tsx index a92b8471b..f13b61295 100644 --- a/src/renderer/src/components/settings/SshTargetForm.tsx +++ b/src/renderer/src/components/settings/SshTargetForm.tsx @@ -7,32 +7,8 @@ import { import { Button } from '../ui/button' import { Input } from '../ui/input' import { Label } from '../ui/label' - -export type EditingTarget = { - label: string - configHost: string - host: string - port: string - username: string - identityFile: string - proxyCommand: string - jumpHost: string - relayGracePeriodSeconds: string - relayKeepAliveUntilReset: boolean -} - -export const EMPTY_FORM: EditingTarget = { - label: '', - configHost: '', - host: '', - port: '22', - username: '', - identityFile: '', - proxyCommand: '', - jumpHost: '', - relayGracePeriodSeconds: String(DEFAULT_SSH_RELAY_GRACE_PERIOD_SECONDS), - relayKeepAliveUntilReset: false -} +import { applyParsedSshHostInput, type EditingTarget } from './ssh-target-draft' +export { EMPTY_FORM, type EditingTarget } from './ssh-target-draft' type SshTargetFormProps = { editingId: string | null @@ -69,15 +45,16 @@ export function SshTargetForm({ />
- + onFormChange((f) => ({ ...f, host: e.target.value }))} - placeholder="192.168.1.100 or server.example.com" + onBlur={() => onFormChange(applyParsedSshHostInput)} + placeholder="server, deploy@server:2222, ssh://server" />
- + onFormChange((f) => ({ ...f, username: e.target.value }))} diff --git a/src/renderer/src/components/settings/ssh-target-draft.test.ts b/src/renderer/src/components/settings/ssh-target-draft.test.ts new file mode 100644 index 000000000..6a703c5c3 --- /dev/null +++ b/src/renderer/src/components/settings/ssh-target-draft.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, it } from 'vitest' +import { + EMPTY_FORM, + applyParsedSshHostInput, + getSshTargetDraftConnectionFields, + parseSshHostInput +} from './ssh-target-draft' + +describe('parseSshHostInput', () => { + it('parses scp-style user, host, and port input', () => { + expect(parseSshHostInput('deploy@example.com:2202')).toEqual({ + host: 'example.com', + username: 'deploy', + port: 2202, + configHost: 'example.com' + }) + }) + + it('parses ssh URLs', () => { + expect(parseSshHostInput('ssh://deploy@example.com:2202/srv/app')).toEqual({ + host: 'example.com', + username: 'deploy', + port: 2202, + configHost: 'example.com' + }) + }) + + it('keeps plain OpenSSH config aliases valid without a username', () => { + expect(parseSshHostInput('prod-box')).toEqual({ + host: 'prod-box', + username: undefined, + port: undefined, + configHost: 'prod-box' + }) + }) +}) + +describe('applyParsedSshHostInput', () => { + it('fills empty username and default port from pasted input', () => { + expect( + applyParsedSshHostInput({ ...EMPTY_FORM, host: 'deploy@example.com:2202' }) + ).toMatchObject({ + host: 'example.com', + configHost: 'example.com', + username: 'deploy', + port: '2202' + }) + }) + + it('does not overwrite explicit username or non-default port', () => { + expect( + applyParsedSshHostInput({ + ...EMPTY_FORM, + host: 'deploy@example.com:2202', + username: 'root', + port: '2022' + }) + ).toMatchObject({ + host: 'example.com', + username: 'root', + port: '2022' + }) + }) +}) + +describe('getSshTargetDraftConnectionFields', () => { + it('uses pasted user and port when the dedicated fields are still default', () => { + expect( + getSshTargetDraftConnectionFields({ ...EMPTY_FORM, host: 'deploy@example.com:2202' }) + ).toEqual({ + host: 'example.com', + configHost: 'example.com', + username: 'deploy', + port: 2202 + }) + }) + + it('allows config aliases without a username', () => { + expect(getSshTargetDraftConnectionFields({ ...EMPTY_FORM, host: 'prod-box' })).toEqual({ + host: 'prod-box', + configHost: 'prod-box', + username: '', + port: 22 + }) + }) +}) diff --git a/src/renderer/src/components/settings/ssh-target-draft.ts b/src/renderer/src/components/settings/ssh-target-draft.ts new file mode 100644 index 000000000..e2e8a605d --- /dev/null +++ b/src/renderer/src/components/settings/ssh-target-draft.ts @@ -0,0 +1,173 @@ +import { + DEFAULT_SSH_RELAY_GRACE_PERIOD_SECONDS, + MAX_SSH_RELAY_GRACE_PERIOD_SECONDS, + MIN_SSH_RELAY_GRACE_PERIOD_SECONDS +} from '../../../../shared/ssh-types' + +export type EditingTarget = { + label: string + configHost: string + host: string + port: string + username: string + identityFile: string + proxyCommand: string + jumpHost: string + relayGracePeriodSeconds: string + relayKeepAliveUntilReset: boolean +} + +export const EMPTY_FORM: EditingTarget = { + label: '', + configHost: '', + host: '', + port: '22', + username: '', + identityFile: '', + proxyCommand: '', + jumpHost: '', + relayGracePeriodSeconds: String(DEFAULT_SSH_RELAY_GRACE_PERIOD_SECONDS), + relayKeepAliveUntilReset: false +} + +export type ParsedSshHostInput = { + host: string + username?: string + port?: number + configHost: string +} + +export function parseSshHostInput(rawInput: string): ParsedSshHostInput | null { + const input = rawInput.trim() + if (!input) { + return null + } + + if (/^ssh:\/\//i.test(input)) { + return parseSshUrl(input) + } + + const atIndex = input.lastIndexOf('@') + const username = atIndex > 0 ? input.slice(0, atIndex).trim() : undefined + const hostPort = atIndex > 0 ? input.slice(atIndex + 1).trim() : input + const parsed = parseHostAndOptionalPort(hostPort) + if (!parsed.host) { + return null + } + + return { + host: parsed.host, + username, + port: parsed.port, + configHost: parsed.host + } +} + +export function applyParsedSshHostInput(draft: EditingTarget): EditingTarget { + const parsed = parseSshHostInput(draft.host) + if (!parsed) { + return draft + } + + return { + ...draft, + host: parsed.host, + configHost: draft.configHost.trim() || parsed.configHost, + username: draft.username.trim() || parsed.username || '', + port: + parsed.port !== undefined && isDefaultPortDraft(draft.port) ? String(parsed.port) : draft.port + } +} + +export function getSshTargetDraftConnectionFields(draft: EditingTarget): { + host: string + configHost: string + username: string + port: number +} { + const parsed = parseSshHostInput(draft.host) + const host = parsed?.host ?? draft.host.trim() + const configHost = draft.configHost.trim() || parsed?.configHost || host + const username = draft.username.trim() || parsed?.username || '' + const parsedPort = parseInt(draft.port, 10) + const port = + parsed?.port !== undefined && isDefaultPortDraft(draft.port) ? parsed.port : parsedPort + + return { + host, + configHost, + username, + port + } +} + +export function parseRelayGracePeriodSeconds(draft: EditingTarget): number { + return draft.relayKeepAliveUntilReset ? 0 : parseInt(draft.relayGracePeriodSeconds, 10) +} + +export function isRelayGracePeriodValid(draft: EditingTarget, graceSeconds: number): boolean { + return ( + draft.relayKeepAliveUntilReset || + (!isNaN(graceSeconds) && + graceSeconds >= MIN_SSH_RELAY_GRACE_PERIOD_SECONDS && + graceSeconds <= MAX_SSH_RELAY_GRACE_PERIOD_SECONDS) + ) +} + +function parseSshUrl(input: string): ParsedSshHostInput | null { + try { + const url = new URL(input) + if (url.protocol !== 'ssh:' || !url.hostname) { + return null + } + const port = url.port ? parseInt(url.port, 10) : undefined + if (port !== undefined && !isValidPort(port)) { + return null + } + return { + host: url.hostname, + username: url.username ? decodeURIComponent(url.username) : undefined, + port, + configHost: url.hostname + } + } catch { + return null + } +} + +function parseHostAndOptionalPort(input: string): { host: string; port?: number } { + if (input.startsWith('[')) { + const closeIndex = input.indexOf(']') + if (closeIndex > 1) { + const host = input.slice(1, closeIndex) + const suffix = input.slice(closeIndex + 1) + if (suffix.startsWith(':')) { + const port = parsePort(suffix.slice(1)) + return port === undefined ? { host } : { host, port } + } + return { host } + } + } + + const portMatch = input.match(/^([^:]+):(\d{1,5})$/) + if (portMatch) { + const port = parsePort(portMatch[2]) + return port === undefined ? { host: input } : { host: portMatch[1], port } + } + + return { host: input } +} + +function parsePort(value: string): number | undefined { + const port = parseInt(value, 10) + return isValidPort(port) ? port : undefined +} + +function isValidPort(port: number): boolean { + return Number.isInteger(port) && port >= 1 && port <= 65535 +} + +function isDefaultPortDraft(value: string): boolean { + const trimmed = value.trim() + return trimmed === '' || trimmed === '22' +}