diff --git a/apps/desktop/electron/connection-registry.test.ts b/apps/desktop/electron/connection-registry.test.ts index 88398cfbec..eb4c025fbe 100644 --- a/apps/desktop/electron/connection-registry.test.ts +++ b/apps/desktop/electron/connection-registry.test.ts @@ -17,12 +17,14 @@ import { labelKey, labelSlug, LOCAL_CONNECTION_ID, + mergeConnectionInput, migrateV1ToRegistry, normalizeConnectionInput, normalizeRegistry, REGISTRY_VERSION, removeConnection, setPrimaryConnection, + uniqueLabel, upsertConnection } from './connection-registry' @@ -57,8 +59,96 @@ test('connectionIdForLabel suffixes on collision and never mints "local"', () => assert.equal(connectionIdForLabel('Local', []), 'local-2') }) +test('uniqueLabel counts up (never "X 2 2") and clamps long candidates', () => { + assert.equal(uniqueLabel('Homelab', []), 'Homelab') + assert.equal(uniqueLabel('Homelab', ['Homelab']), 'Homelab 2') + assert.equal(uniqueLabel('Homelab', ['Homelab', 'Homelab 2']), 'Homelab 3') + // Case-insensitive collision detection. + assert.equal(uniqueLabel('homelab', ['HOMELAB']), 'homelab 2') + + const long = 'x'.repeat(300) + assert.ok(uniqueLabel(long, []).length <= 64) + assert.ok(uniqueLabel(long, [uniqueLabel(long, [])]).length <= 64) +}) + // --- normalizeConnectionInput --- +test('save rejects the reserved "local" id on non-local kinds', () => { + assert.throws( + () => normalizeConnectionInput({ id: 'local', kind: 'remote', label: 'Sneaky', url: 'http://x:1' }, emptyRegistry()), + /reserved/ + ) +}) + +test('token only persists on token-auth remotes; oauth/cloud drop it', () => { + const registry = emptyRegistry() + + const tokenAuth = normalizeConnectionInput( + { kind: 'remote', label: 'A', url: 'http://a:1', authMode: 'token', token: { enc: 'x' } }, + registry + ) + + assert.deepEqual(tokenAuth.token, { enc: 'x' }) + + const oauth = normalizeConnectionInput( + { kind: 'remote', label: 'B', url: 'http://b:1', authMode: 'oauth', token: { enc: 'x' } }, + registry + ) + + assert.equal(oauth.token, undefined) + + const cloud = normalizeConnectionInput( + { kind: 'cloud', label: 'C', url: 'https://c.hermes.cloud', authMode: 'oauth', token: { enc: 'x' } }, + registry + ) + + assert.equal(cloud.token, undefined) +}) + +// --- mergeConnectionInput (edit inheritance) --- + +test('merge preserves fields the editor does not carry (org, ssh extras)', () => { + const cloud = { authMode: 'oauth' as const, id: 'c', kind: 'cloud' as const, label: 'Cloud', org: 'nous', url: 'https://a.cloud' } + const renamed = mergeConnectionInput({ id: 'c', kind: 'cloud', label: 'Renamed', url: 'https://a.cloud' }, cloud) + + assert.equal(renamed.org, 'nous') + + const ssh = { + host: 'homelab.lan', + id: 's', + keyPath: '/k/id', + kind: 'ssh' as const, + label: 'Box', + port: 2222, + remoteHermesPath: '/opt/hermes', + remoteProfile: 'research', + user: 'k' + } + + const labelOnly = mergeConnectionInput({ id: 's', kind: 'ssh', label: 'Renamed box' }, ssh) + + assert.equal(labelOnly.remoteHermesPath, '/opt/hermes') + assert.equal(labelOnly.remoteProfile, 'research') + assert.equal(labelOnly.host, 'homelab.lan') + assert.equal(labelOnly.user, 'k') + assert.equal(labelOnly.port, 2222) +}) + +test('merge: a supplied ssh host string beats stored user/port', () => { + const ssh = { host: 'spark1', id: 's', kind: 'ssh' as const, label: 'Spark', port: 2222, user: 'tek' } + const merged = mergeConnectionInput({ host: 'admin@newbox:2200', id: 's', kind: 'ssh', label: 'Spark' }, ssh) + + // Stored user/port must NOT ride along — the host string is authoritative. + assert.equal(merged.user, undefined) + assert.equal(merged.port, undefined) + + const entry = normalizeConnectionInput(merged, emptyRegistry()) + + assert.equal(entry.host, 'newbox') + assert.equal(entry.user, 'admin') + assert.equal(entry.port, 2200) +}) + test('save rejects a missing label with a device-name message', () => { assert.throws( () => normalizeConnectionInput({ kind: 'remote', label: ' ', url: 'http://10.0.0.5:9119' }, emptyRegistry()), diff --git a/apps/desktop/electron/connection-registry.ts b/apps/desktop/electron/connection-registry.ts index f1fca1ff17..7f4e33b3e6 100644 --- a/apps/desktop/electron/connection-registry.ts +++ b/apps/desktop/electron/connection-registry.ts @@ -78,6 +78,33 @@ export function labelKey(label: string): string { .toLowerCase() } +/** + * Derive a registry-unique label from a candidate: clamps to LABEL_MAX (a + * migrated URL host can exceed it, which would fail validation on any later + * edit) and suffixes " 2" / " 3" / … on collision. The single home of the + * label-dedup rule — normalizeRegistry and the migration both use it. + */ +export function uniqueLabel(candidate: string, taken: Iterable): string { + const used = new Set([...taken].map(labelKey)) + + // Reserve room for a collision suffix so the suffixed form stays in-bounds. + const base = String(candidate || '') + .trim() + .slice(0, LABEL_MAX - 4) + + if (!used.has(labelKey(base))) { + return base + } + + for (let n = 2; ; n += 1) { + const suffixed = `${base} ${n}` + + if (!used.has(labelKey(suffixed))) { + return suffixed + } + } +} + /** Kebab-slug of a label for ids and @handles. Never empty for a non-empty label. */ export function labelSlug(label: string): string { const slug = String(label || '') @@ -168,6 +195,15 @@ export function normalizeConnectionInput(input: ConnectionInput, registry: Conne return { id: LOCAL_CONNECTION_ID, kind: 'local', label } } + // The reserved local id can never be claimed by a non-local entry — a + // crafted IPC payload ({id:'local', kind:'remote', …}) would otherwise + // replace the local entry via upsert and break the exactly-one-local + // invariant. connectionIdForLabel never mints 'local'; reject it when + // supplied, too. + if (input.id === LOCAL_CONNECTION_ID) { + throw new Error('The id "local" is reserved for the local connection.') + } + const id = input.id || connectionIdForLabel(label, registry.connections.map(c => c.id)) if (kind === 'ssh') { @@ -196,7 +232,11 @@ export function normalizeConnectionInput(input: ConnectionInput, registry: Conne const authMode = normAuthMode(input.authMode) const entry: RegistryConnection = { id, kind, label, url, authMode } - if (input.token !== undefined) { + // A token is only meaningful for token-auth remotes. Dropping it here is + // what clears the stale envelope when an entry is switched token→oauth + // (or is a cloud entry, which authenticates via the portal session) — + // otherwise dead secret material rides along on the edited entry. + if (input.token !== undefined && kind === 'remote' && authMode === 'token') { entry.token = input.token } @@ -212,6 +252,50 @@ export function normalizeConnectionInput(input: ConnectionInput, registry: Conne throw new Error(`Unknown connection kind: ${String(kind)}`) } +/** + * Merge a (possibly partial) edit payload over the stored entry so fields the + * editor doesn't carry survive a save. Renaming a migrated cloud entry must + * not drop its `org` (downstream update-fanout uses it to skip + * platform-managed instances), and renaming an ssh entry must not drop + * `remoteHermesPath`/`remoteProfile`. Only fields the payload explicitly + * carries (non-undefined) override; `token` is deliberately NOT merged here — + * the caller owns secret handling. + */ +export function mergeConnectionInput(input: ConnectionInput, existing?: null | RegistryConnection): ConnectionInput { + if (!existing || existing.kind !== input.kind) { + return input + } + + const merged: ConnectionInput = { ...input } + + const inherit = (field: keyof ConnectionInput & keyof RegistryConnection) => { + if (merged[field] === undefined && existing[field] !== undefined) { + ;(merged as unknown as Record)[field] = existing[field] + } + } + + inherit('url') + inherit('authMode') + inherit('org') + inherit('host') + inherit('keyPath') + inherit('remoteHermesPath') + inherit('remoteProfile') + + // ssh user/port: the editor shows ONE composite host field (user@host:port), + // and normalizeSshConfig gives explicit user/port fields precedence over the + // parsed host string. Inheriting stored user/port alongside a NEW host string + // would resurrect the old values over what the user just typed — so when the + // payload carries a host, the host string is authoritative and stored + // user/port are NOT inherited. + if (input.host === undefined || !String(input.host).trim()) { + inherit('user') + inherit('port') + } + + return merged +} + // ── Registry-level operations (all pure: return a new registry) ──────────── function localEntry(label = 'This device'): RegistryConnection { @@ -253,9 +337,7 @@ export function normalizeRegistry(raw: unknown): ConnectionRegistry { kind === 'ssh' ? String(entry.host || 'ssh') : hostLabelFromBaseUrl(String(entry.url || '')) || String(kind) } - while (seenLabels.has(labelKey(label))) { - label = `${label} 2` - } + label = uniqueLabel(label, seenLabels) let id = kind === 'local' ? LOCAL_CONNECTION_ID : String(entry.id || '').trim() @@ -348,11 +430,10 @@ export function migrateV1ToRegistry(v1: unknown): ConnectionRegistry { return existing } - let label = hostLabelFromBaseUrl(url) || (kind === 'cloud' ? 'Hermes Cloud' : 'Remote gateway') - - while (connections.some(c => labelKey(c.label) === labelKey(label))) { - label = `${label} 2` - } + const label = uniqueLabel( + hostLabelFromBaseUrl(url) || (kind === 'cloud' ? 'Hermes Cloud' : 'Remote gateway'), + connections.map(c => c.label) + ) const entry: RegistryConnection = { id: connectionIdForLabel(label, connections.map(c => c.id)), @@ -392,11 +473,7 @@ export function migrateV1ToRegistry(v1: unknown): ConnectionRegistry { return existing } - let label = ssh.host - - while (connections.some(c => labelKey(c.label) === labelKey(label))) { - label = `${label} 2` - } + const label = uniqueLabel(ssh.host, connections.map(c => c.label)) const { mode: _mode, ...sshFields } = ssh diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 0af5708316..52b28ff6c2 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -84,6 +84,7 @@ import { tokenPreview } from './connection-config' import { + mergeConnectionInput, migrateV1ToRegistry, normalizeConnectionInput, normalizeRegistry, @@ -7590,9 +7591,21 @@ function readDesktopConnectionsRegistry() { if (mtime === null) { // First run on this build: import the v1 single-connection config. The v1 - // file is NOT modified or deleted — older builds keep reading it. + // file is NOT modified or deleted — older builds keep reading it. The + // migration is deterministic over the v1 input, so even if two processes + // race the first run (updater relaunch, second window), both derive the + // same registry and the later atomic write is a no-op content-wise. registry = migrateV1ToRegistry(readDesktopConnectionConfig()) - writeDesktopConnectionsRegistry(registry) + + try { + writeDesktopConnectionsRegistry(registry) + } catch { + // Write failed (full disk, read-only userData). Keep the migrated + // registry in memory so list/save keep working this session instead of + // hard-failing every hermes:connections:* call. + connectionRegistryCache = registry + connectionRegistryCacheMtime = null + } return connectionRegistryCache } @@ -7638,18 +7651,33 @@ function sanitizeRegistryConnection(entry) { } function sanitizeConnectionsRegistry(registry = readDesktopConnectionsRegistry()) { + // Same keyring probe the v1 sanitize exposes: lets the Connections panel + // offer the plain-text opt-in on keyring-less Linux instead of failing. + let secureTokenStorage = false + + try { + secureTokenStorage = Boolean(safeStorage.isEncryptionAvailable()) + } catch { + secureTokenStorage = false + } + return { version: registry.version, primary: registry.primary, + secureTokenStorage, connections: registry.connections.map(sanitizeRegistryConnection) } } /** * Save (create or edit) a registry connection from a renderer payload. - * Token handling mirrors coerceDesktopConnectionConfig: an incoming plaintext - * token is encrypted (with the same plain-text opt-in seam); an absent token - * field inherits the stored envelope on edit. + * Edits merge over the stored entry (mergeConnectionInput) so fields the + * editor doesn't carry — cloud `org`, ssh `remoteHermesPath`/`remoteProfile` — + * survive a rename. Token handling mirrors coerceDesktopConnectionConfig: an + * incoming plaintext token is encrypted (honoring the same allowPlainTextToken + * opt-in seam as Settings → Gateway); an absent token field inherits the + * stored envelope on edit; switching auth away from 'token' clears it + * (normalizeConnectionInput drops tokens on non-token entries). */ function saveRegistryConnection(input: any = {}) { const registry = readDesktopConnectionsRegistry() @@ -7664,7 +7692,8 @@ function saveRegistryConnection(input: any = {}) { encryptSecret: encryptDesktopSecret }) - const entry = normalizeConnectionInput({ ...input, token }, registry) + const merged = mergeConnectionInput({ ...input, token }, existing) + const entry = normalizeConnectionInput(merged, registry) // Token-auth remotes must actually have a token to be dialable. OAuth and // cloud entries authenticate via cookies/native tokens instead. @@ -11352,12 +11381,8 @@ ipcMain.handle('hermes:connections:test', async (_event, id) => { throw new Error(`No connection with id "${String(id || '')}".`) } - // Reuse the existing probe stack by mapping the registry entry onto the - // settings-payload shape testDesktopConnectionConfig already understands. - if (entry.kind === 'local') { - return testDesktopConnectionConfig({ mode: 'local' }) - } - + // The ssh probe path in testDesktopConnectionConfig never consults v1 + // connection state, so mapping the entry onto it is safe. if (entry.kind === 'ssh') { return testDesktopConnectionConfig({ mode: 'ssh', @@ -11369,13 +11394,53 @@ ipcMain.handle('hermes:connections:test', async (_event, id) => { }) } - return testDesktopConnectionConfig({ - mode: entry.kind, - remoteUrl: entry.url, - remoteAuthMode: entry.authMode, - remoteToken: decryptDesktopSecret(entry.token) || undefined, - cloudOrg: entry.org - }) + // Remote/cloud/local probe built DIRECTLY from the registry entry. Routing + // through coerceDesktopConnectionConfig would use v1 connection.json as the + // `existing` base: an entry with a broken/absent token would inherit the v1 + // global remote's token and send it to THIS entry's URL (cross-host + // credential transmission + a false "reachable"), and testing the local + // entry would probe whatever v1's global mode points at instead of the + // app-managed local backend. + let baseUrl + let token = null + let authMode = 'token' + + if (entry.kind === 'local') { + const local = await startHermes() + baseUrl = local.baseUrl + token = local.token + authMode = normAuthMode(local.authMode) + } else { + baseUrl = normalizeRemoteBaseUrl(entry.url) + authMode = normAuthMode(entry.authMode) + + if (authMode !== 'oauth') { + token = decryptDesktopSecret(entry.token) + + if (!token) { + throw new Error('This connection has no saved session token. Edit the connection and paste one.') + } + } + } + + const status = (await fetchJson(`${baseUrl}/api/status`, token, { timeoutMs: 8_000 })) as any + + // Same HTTP+WS two-leg check as testDesktopConnectionConfig: HTTP alone is + // a false positive when the WebSocket leg is blocked. + const wsUrl = await resolveTestWsUrl(baseUrl, authMode, token, { mintTicket: mintGatewayWsTicket }) + + if (wsUrl && typeof globalThis.WebSocket === 'function') { + const probe = await probeGatewayWebSocket(wsUrl, { WebSocketImpl: globalThis.WebSocket }) + + if (!probe.ok) { + throw new Error( + `Reached the gateway over HTTP, but the live WebSocket (/api/ws) connection failed: ${probe.reason} ` + + 'The HTTP check can pass while the WebSocket is blocked by a proxy, firewall, or gateway auth/origin guard.' + ) + } + } + + return { ok: true, baseUrl, version: status?.version || null } }) ipcMain.handle('hermes:connection-config:probe', async (_event, rawUrl) => probeRemoteAuthMode(rawUrl)) ipcMain.handle('hermes:connection-config:oauth-login', async (_event, rawUrl) => { diff --git a/apps/desktop/src/app/settings/connections-settings.test.tsx b/apps/desktop/src/app/settings/connections-settings.test.tsx index aaaba36d1b..a5c4ba2006 100644 --- a/apps/desktop/src/app/settings/connections-settings.test.tsx +++ b/apps/desktop/src/app/settings/connections-settings.test.tsx @@ -25,6 +25,7 @@ const registry: DesktopConnectionsRegistry = { } ], primary: 'local', + secureTokenStorage: true, version: 2 } diff --git a/apps/desktop/src/app/settings/connections-settings.tsx b/apps/desktop/src/app/settings/connections-settings.tsx index ef793cd1ea..9f504b2ef8 100644 --- a/apps/desktop/src/app/settings/connections-settings.tsx +++ b/apps/desktop/src/app/settings/connections-settings.tsx @@ -32,8 +32,6 @@ interface EditorState { authMode: 'oauth' | 'token' token: string host: string - user: string - port: string keyPath: string } @@ -45,15 +43,18 @@ function editorFromConnection(conn: DesktopRegistryConnection): EditorState { url: conn.url || '', authMode: conn.authMode || 'token', token: '', - host: conn.host || '', - user: conn.user || '', - port: conn.port ? String(conn.port) : '', + // Reconstruct the composite the single ssh host field displays. The save + // payload sends ONLY this string (never separate user/port), because + // normalizeSshConfig gives explicit user/port fields precedence over the + // parsed host string — sending stored user/port alongside a retyped host + // would silently resurrect the old values. + host: conn.host ? `${conn.user ? `${conn.user}@` : ''}${conn.host}${conn.port ? `:${conn.port}` : ''}` : '', keyPath: conn.keyPath || '' } } function emptyEditor(kind: DesktopConnectionKind): EditorState { - return { id: null, kind, label: '', url: '', authMode: 'token', token: '', host: '', user: '', port: '', keyPath: '' } + return { id: null, kind, label: '', url: '', authMode: 'token', token: '', host: '', keyPath: '' } } /** @@ -72,6 +73,7 @@ export function ConnectionsSettings() { const [busyId, setBusyId] = useState(null) const [testingId, setTestingId] = useState(null) const [removeTarget, setRemoveTarget] = useState(null) + const [plainTextConfirm, setPlainTextConfirm] = useState(false) const bridge = window.hermesDesktop?.connections @@ -97,46 +99,69 @@ export function ConnectionsSettings() { void load() }, [load]) - const save = useCallback(async () => { - if (!bridge || !editor) { - return - } - - setSaving(true) - - try { - const payload: DesktopRegistryConnectionInput = { - kind: editor.kind, - label: editor.label + const save = useCallback( + async (allowPlainTextToken = false) => { + if (!bridge || !editor) { + return } - if (editor.id) { - payload.id = editor.id - } + setSaving(true) - if (editor.kind === 'remote' || editor.kind === 'cloud') { - payload.url = editor.url - payload.authMode = editor.authMode - - if (editor.token.trim()) { - payload.token = editor.token.trim() + try { + const payload: DesktopRegistryConnectionInput = { + kind: editor.kind, + label: editor.label } - } else if (editor.kind === 'ssh') { - payload.host = editor.host - payload.user = editor.user || undefined - payload.port = editor.port.trim() ? Number(editor.port) : null - payload.keyPath = editor.keyPath || undefined - } - const result = await bridge.save(payload) - setRegistry(result.registry) - setEditor(null) - } catch (err) { - notifyError(err, s.saveFailed) - } finally { - setSaving(false) - } - }, [bridge, editor, s.saveFailed]) + if (editor.id) { + payload.id = editor.id + } + + if (editor.kind === 'remote' || editor.kind === 'cloud') { + payload.url = editor.url + payload.authMode = editor.authMode + + if (editor.token.trim()) { + payload.token = editor.token.trim() + } + + if (allowPlainTextToken) { + payload.allowPlainTextToken = true + } + } else if (editor.kind === 'ssh') { + // The composite host string (user@host:port) is the single source + // of truth — never send separate user/port (see editorFromConnection). + payload.host = editor.host + payload.keyPath = editor.keyPath || undefined + } + + const result = await bridge.save(payload) + setRegistry(result.registry) + setEditor(null) + setPlainTextConfirm(false) + } catch (err) { + // Keyring-less machine and the user hasn't consented to plain-text + // storage yet: raise the same opt-in dialog Settings → Gateway uses + // instead of dead-ending the save. + if ( + !allowPlainTextToken && + registry?.secureTokenStorage === false && + editor.kind === 'remote' && + editor.authMode === 'token' && + editor.token.trim() + ) { + setPlainTextConfirm(true) + + return + } + + notifyError(err, s.saveFailed) + } finally { + setSaving(false) + } + }, + [bridge, editor, registry?.secureTokenStorage, s.saveFailed] + ) const remove = useCallback(async () => { if (!bridge || !removeTarget) { @@ -191,7 +216,7 @@ export function ConnectionsSettings() { if (reachable) { notify({ title: conn.label, message: s.testOk }) } else { - notifyError(new Error(result.error || s.testFailed), conn.label) + notifyError(new Error(result.error || conn.label), s.testFailed) } } catch (err) { notifyError(err, s.testFailed) @@ -216,7 +241,12 @@ export function ConnectionsSettings() { return ( -

{s.intro}

+

{s.intro}

+ {/* Storage-only slice: be explicit that routing consumption is staged so + "Make primary" isn't read as an immediate connection switch. */} +

+ {s.stagedNote} +

{!registry || registry.connections.length === 0 ? ( @@ -293,7 +323,11 @@ export function ConnectionsSettings() { {editor ? (
- {(['remote', 'cloud', 'ssh'] as const).map(kind => ( + {/* Cloud creation is deliberately absent: a dialable cloud entry + comes from the Hermes Cloud sign-in/discovery flow (Settings → + Gateway), not a hand-typed URL. Migrated/discovered cloud + entries remain editable (kind buttons are disabled on edit). */} + {(editor.kind === 'cloud' ? (['cloud'] as const) : (['remote', 'ssh'] as const)).map(kind => (