From ee667cdc2bed2035dd359ea91631b21d64aa364e Mon Sep 17 00:00:00 2001 From: Bill Date: Wed, 7 Oct 2026 20:44:28 +0800 Subject: [PATCH] fix(main): harden startup chain, updater/sftp timeouts, host-key guard, shell env scrub --- src/main/globalShortcuts.ts | 31 +++++++++++ src/main/index.ts | 106 ++++++++++++++++++++++-------------- src/main/pty.ts | 8 +++ src/main/sftp.ts | 57 +++++++++++++++---- src/main/ssh.ts | 8 +++ src/main/updater.ts | 46 +++++++++++++--- 6 files changed, 197 insertions(+), 59 deletions(-) diff --git a/src/main/globalShortcuts.ts b/src/main/globalShortcuts.ts index a05e68a..807a777 100644 --- a/src/main/globalShortcuts.ts +++ b/src/main/globalShortcuts.ts @@ -1,9 +1,33 @@ import { BrowserWindow, globalShortcut } from 'electron' +/** + * Chords the app owns outright: Ctrl+L is the panic lock, captured in the main + * window's before-input-event. The settings recorder (SettingsTabs.tsx, + * RESERVED_EXACT_ACCELERATORS) refuses to save it, but that guard only covers + * values entered after it existed — a shortcut persisted by an older build + * still arrives here, and a global registration intercepts the key at the OS + * level even while the window is focused, silently killing the lock shortcut. + * Normalized (modifier aliases + case folded) so every spelling is caught. + */ +const RESERVED_ACCELERATORS = new Set(['control+l', 'commandorcontrol+l']) + +function normalizeAccelerator(accelerator: string): string { + return accelerator + .split('+') + .map((part) => { + const p = part.trim().toLowerCase() + if (p === 'ctrl') return 'control' + if (p === 'cmdorctrl' || p === 'commandorctrl') return 'commandorcontrol' + return p + }) + .join('+') +} + /** * Register the global show/hide toggle for the main window. * * - accelerator '' / undefined => disabled (no global key bound). + * - A reserved chord (Ctrl+L) is skipped: see RESERVED_ACCELERATORS. * - Passing an invalid accelerator string makes Electron's register() throw; * we swallow that here so a bad user-supplied value never crashes the app. * - register() returning false means the accelerator is already taken by @@ -15,6 +39,13 @@ export function applyGlobalShortcut(accelerator: string | undefined): void { globalShortcut.unregisterAll() if (!accelerator) return + if (RESERVED_ACCELERATORS.has(normalizeAccelerator(accelerator))) { + console.warn( + `[global-shortcut] "${accelerator}" is reserved for the Ctrl+L lock shortcut; not registering` + ) + return + } + const handler = (): void => { const win = BrowserWindow.getAllWindows()[0] if (!win || win.isDestroyed()) return diff --git a/src/main/index.ts b/src/main/index.ts index d0faf80..d9f93ab 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -1,4 +1,4 @@ -import { app, BrowserWindow, globalShortcut, net, nativeImage, protocol, shell } from 'electron' +import { app, BrowserWindow, dialog, globalShortcut, net, nativeImage, protocol, shell } from 'electron' import { existsSync } from 'fs' import { join } from 'path' import { pathToFileURL } from 'url' @@ -12,7 +12,7 @@ import { isLockBlockedShortcut, isPanicLockChord } from './lockShortcuts' import { initTray, markQuitting, onMainWindowClose, refreshTrayMenu } from './tray' import { configureAutoUpdater, registerUpdateIpc } from './updater' import { applyWindowChrome } from './windowChrome' -import { onLanguageChange } from '@shared/i18n' +import { onLanguageChange, t } from '@shared/i18n' import { getThemeById } from '@shared/theme' /** Re-create the main window (tray restore path after all windows are gone). */ @@ -69,45 +69,63 @@ if (!gotSingleInstanceLock) { } else { app.on('second-instance', () => showOrCreate()) - app.whenReady().then(() => { - // Serve the configured background image (path lives in settings; anything - // else — including a path that is no longer configured — is refused, so the - // protocol cannot be used to read arbitrary files). - protocol.handle('otimg', (request) => { - const url = new URL(request.url) - const requested = decodeURIComponent(url.pathname.replace(/^\//, '')) - const allowed = loadSettings().terminal.backgroundImage - if (allowed === '' || requested !== allowed || !existsSync(requested)) { - return new Response('', { status: 403 }) + app + .whenReady() + .then(() => { + // Serve the configured background image (path lives in settings; anything + // else — including a path that is no longer configured — is refused, so the + // protocol cannot be used to read arbitrary files). + protocol.handle('otimg', (request) => { + const url = new URL(request.url) + const requested = decodeURIComponent(url.pathname.replace(/^\//, '')) + const allowed = loadSettings().terminal.backgroundImage + if (allowed === '' || requested !== allowed || !existsSync(requested)) { + return new Response('', { status: 403 }) + } + return net.fetch(pathToFileURL(requested).toString()) + }) + + registerIpc() + registerUpdateIpc() + // Before the window exists: a renderer-triggered check must not run against + // the updater's defaults (feed, proxy, autoDownload are set in here). + configureAutoUpdater() + // OS-level effects (login item, sleep blocker) must apply even if the + // settings dialog is never opened this run. + applyStartupSystemSettings(loadSettings()) + // The lock state has to exist before the window loads, so a lockAtStartup + // lock is already in place when the renderer asks for it. It also starts the + // idle watcher, which is why it belongs after ready: powerMonitor cannot be + // touched before that. A restored startup lock engages before any publish, + // so the menu teardown is applied here from the flag itself. + const lock = initLockController() + applyMenuLockState(lock.isLocked()) + createWindow() + initTray(showOrCreate) + // Tray labels are resolved from the dictionary at build time, so the menu has + // to be rebuilt whenever the interface language changes. + onLanguageChange(() => refreshTrayMenu()) + + app.on('activate', () => { + if (BrowserWindow.getAllWindows().length === 0) createWindow() + }) + }) + // A throw anywhere above (protocol, IPC, updater, lock controller, window, + // tray) used to leave the process running with the single-instance lock held + // and neither a window nor a tray to reach it — a zombie that refuses every + // later launch. Report and exit so a relaunch can retry from scratch. + .catch((err: unknown) => { + console.error('[main] startup failed:', err) + try { + dialog.showErrorBox( + 'OpenTerminal', + t('main.startupFailed', { detail: err instanceof Error ? err.message : String(err) }) + ) + } catch { + // no display / dialog unavailable: the console line is all we have } - return net.fetch(pathToFileURL(requested).toString()) + app.exit(1) }) - - registerIpc() - registerUpdateIpc() - // Before the window exists: a renderer-triggered check must not run against - // the updater's defaults (feed, proxy, autoDownload are set in here). - configureAutoUpdater() - // OS-level effects (login item, sleep blocker) must apply even if the - // settings dialog is never opened this run. - applyStartupSystemSettings(loadSettings()) - // The lock state has to exist before the window loads, so a lockAtStartup - // lock is already in place when the renderer asks for it. It also starts the - // idle watcher, which is why it belongs after ready: powerMonitor cannot be - // touched before that. A restored startup lock engages before any publish, - // so the menu teardown is applied here from the flag itself. - const lock = initLockController() - applyMenuLockState(lock.isLocked()) - createWindow() - initTray(showOrCreate) - // Tray labels are resolved from the dictionary at build time, so the menu has - // to be rebuilt whenever the interface language changes. - onLanguageChange(() => refreshTrayMenu()) - - app.on('activate', () => { - if (BrowserWindow.getAllWindows().length === 0) createWindow() - }) - }) } function createWindow(): void { @@ -213,11 +231,17 @@ function createWindow(): void { // ELECTRON_RENDERER_URL is honored in dev builds only — a packaged build must // always load the bundled renderer file, no matter what the environment says. + // Both loads are fire-and-forget, so the rejection is surfaced explicitly + // instead of dying as a floating promise. const devUrl = devRendererUrl() if (devUrl) { - win.loadURL(devUrl) + void win.loadURL(devUrl).catch((err) => { + console.error('[main] loadURL failed:', err) + }) } else { - win.loadFile(join(__dirname, '../renderer/index.html')) + void win.loadFile(join(__dirname, '../renderer/index.html')).catch((err) => { + console.error('[main] loadFile failed:', err) + }) } } diff --git a/src/main/pty.ts b/src/main/pty.ts index 5b7a40d..4955dbc 100644 --- a/src/main/pty.ts +++ b/src/main/pty.ts @@ -189,6 +189,14 @@ export function createPty(opts: PtyCreateOptions = {}, owner?: number): PtyCreat // explicit FORCE_COLOR=0 form of the same request. delete env.NO_COLOR if (env.FORCE_COLOR === '0') delete env.FORCE_COLOR + // Electron's own plumbing must not leak into the user's shell either: + // ELECTRON_RUN_AS_NODE would turn this app's own exe into plain node, and + // NODE_OPTIONS would be applied to every node process started from here. + // OT_UPDATE_TOKEN is the updater credential — the updater is the only thing + // that has a reason to see it, not every shell the user opens. + delete env.ELECTRON_RUN_AS_NODE + delete env.NODE_OPTIONS + delete env.OT_UPDATE_TOKEN // Shell integration (opt-in): let the shell announce its own cwd over OSC 7 so // the remembered path is exact instead of inferred from typed `cd` commands. diff --git a/src/main/sftp.ts b/src/main/sftp.ts index 1144176..116f3a7 100644 --- a/src/main/sftp.ts +++ b/src/main/sftp.ts @@ -52,12 +52,18 @@ export function formatMode(mode: number): string { } /** - * One SFTP channel PER SESSION, cached. Strict sshd builds (MaxSessions 2-3, - * e.g. hardened cloud images) refuse extra channels and drop the whole - * connection when every operation opens its own subsystem. + * One SFTP channel PER SESSION, cached for the session's whole lifetime. Strict + * sshd builds (MaxSessions 2-3, e.g. hardened cloud images) refuse extra + * channels and drop the whole connection when every operation opens its own + * subsystem. Only the session ending (pty.ts) or the channel itself erroring + * drops it: a finished transfer must NOT close it, or the next transfer pays + * for another subsystem. */ const sftpCache = new Map() +/** How long an SFTP subsystem open may take before the client counts as dead. */ +const SFTP_OPEN_TIMEOUT_MS = 10_000 + /** * Our own "session is gone" error. `isTransportError` classifies on the class, * not on the message text: the message is translated, the classifier must not @@ -65,6 +71,13 @@ const sftpCache = new Map() */ class SessionGoneError extends Error {} +/** + * Our own "the client stopped answering" error: an SFTP subsystem open that + * outlived its budget. Transport-classified, so withSftp evicts the possibly + * half-dead channel and retries once on a fresh one. + */ +class SftpTimeoutError extends Error {} + /** * Our own "operation failed for a non-transport reason" error (server-side * failure summary, user cancellation). isTransportError rejects these outright: @@ -82,7 +95,23 @@ async function sftpOf(sessionId: string): Promise { throw new SessionGoneError(t('main.sftp.sessionGone')) } const sftp = await new Promise((resolve, reject) => { - client.sftp((err, sftp_) => (err != null ? reject(err) : resolve(sftp_))) + // A half-dead client can accept the subsystem request and then never call + // back; bound the wait (like execQuiet does) so withSftp can evict and retry. + const timer = setTimeout( + () => reject(new SftpTimeoutError(t('main.sftp.openTimeout'))), + SFTP_OPEN_TIMEOUT_MS + ) + try { + client.sftp((err, sftp_) => { + clearTimeout(timer) + if (err != null) reject(err) + else resolve(sftp_) + }) + } catch (err) { + // ssh2 throws synchronously when the socket is already gone + clearTimeout(timer) + reject(err) + } }) // ssh2.d.ts declares only the used surface; the wrapper is an EventEmitter ;(sftp as SFTPWrapper & { on(event: 'error', cb: (err: Error) => void): void }).on('error', (err: Error) => { @@ -109,14 +138,16 @@ function evictSftp(sessionId: string): void { * another subsystem channel, which strict sshd (MaxSessions 2) punishes by * dropping the whole connection. Only transport-level death retries. * - * Classification is class-based for our own errors (SessionGoneError / - * OperationError) and matches only the ssh2 library's own message strings for - * the rest: ssh2 is English-only and never translated, so the decision cannot - * drift with the UI locale (our translated messages arrive as OperationError - * and are rejected before any text is inspected). + * Classification is class-based for our own errors (SessionGoneError and + * SftpTimeoutError are transport-level, OperationError is not) and matches only + * the ssh2 library's own message strings for the rest: ssh2 is English-only and + * never translated, so the decision cannot drift with the UI locale (our + * translated messages arrive as OperationError and are rejected before any text + * is inspected). */ function isTransportError(err: unknown): boolean { if (err instanceof SessionGoneError) return true + if (err instanceof SftpTimeoutError) return true if (err instanceof OperationError) return false const msg = (err as Error | undefined)?.message ?? '' return /not connected|ECONNRESET|EPIPE|timed out|disconnected|channel|read past end|no response/i.test(msg) @@ -399,8 +430,10 @@ export function uploadRemote( error: (err as Error).message }) } finally { + // The cached channel belongs to the session, not to this transfer: only + // the session ending or the channel erroring drops it (see the cache note + // at the top of the file). activeTransfers.delete(id) - closeSftp(sessionId) } })() @@ -472,8 +505,10 @@ export function downloadRemote( error: (err as Error).message }) } finally { + // The cached channel belongs to the session, not to this transfer: only + // the session ending or the channel erroring drops it (see the cache note + // at the top of the file). activeTransfers.delete(id) - closeSftp(sessionId) } })() diff --git a/src/main/ssh.ts b/src/main/ssh.ts index 22a5bd2..86dbf52 100644 --- a/src/main/ssh.ts +++ b/src/main/ssh.ts @@ -144,6 +144,14 @@ export async function connectSsh( promptUser(conn.host, conn.port, fingerprint, status, timeouts.prompt, deps) .then((accepted) => { + // The prompt can outlive the handshake: the connect timer is re-armed + // only after the decision, but a socket error meanwhile may have failed + // the attempt (and destroyed the client). Pinning the key then would + // record trust for a connection that never completed — drop the answer. + if (settled) { + console.warn('[ssh] host key decision arrived after the handshake failed; ignored') + return + } if (accepted) { try { deps.knownHosts.accept(conn.host, conn.port, hostKey, fingerprint) diff --git a/src/main/updater.ts b/src/main/updater.ts index b8e4e82..2f9b9c0 100644 --- a/src/main/updater.ts +++ b/src/main/updater.ts @@ -24,6 +24,10 @@ const GITHUB_RELEASES_API = const GITHUB_PROBE_URL = 'https://api.github.com/repos/billowliu2/OpenTerminal/releases/latest' const GITHUB_PROBE_TIMEOUT_MS = 20_000 +/** Overall budget for ONE check attempt — see withTimeout. */ +const CHECK_TIMEOUT_MS = 30_000 +/** Changelog / releases-API fetches: a stalled response must not hang the About tab. */ +const FETCH_TIMEOUT_MS = 15_000 let state: UpdateState = { status: 'idle', currentVersion: app.getVersion() } let activeFeed: 'gitea' | 'github' = 'gitea' @@ -75,13 +79,31 @@ function wireEvents(): void { ) } +/** + * Bound one updater call with an overall timeout. electron-updater's own socket + * idle timeout only fires on silence, so a slow trickling response can hold an + * attempt — and the "checking" state the UI shows — open indefinitely. + */ +function withTimeout(promise: Promise, ms: number): Promise { + let timer: NodeJS.Timeout | undefined + const timeout = new Promise((_, reject) => { + const handle = setTimeout(() => reject(new Error(t('main.updater.checkTimeout'))), ms) + handle.unref?.() + timer = handle + }) + return Promise.race([promise, timeout]).finally(() => { + if (timer) clearTimeout(timer) + }) +} + /** * Check updates: GitHub releases first (system proxy, gated by a * connectivity probe), domestic Gitea generic package as fallback. * - * The feed is decided BEFORE touching the updater: checkForUpdates has no - * timeout, and racing it while mid-flight risks two concurrent checks - * polluting state. + * The feed is decided BEFORE touching the updater: a check abandoned on timeout + * cannot be cancelled, and electron-updater de-dupes concurrent checks by + * handing back the in-flight promise — so the fallback attempt below shares the + * abandoned check's fate instead of racing a second request. */ async function checkWithFallback(): Promise { // The feed choice is per attempt: a transient GitHub failure must not pin @@ -91,7 +113,7 @@ async function checkWithFallback(): Promise { if (await probeGithub()) { useFeed('github') try { - await autoUpdater.checkForUpdates() + await withTimeout(autoUpdater.checkForUpdates(), CHECK_TIMEOUT_MS) return } catch (err) { console.warn('[updater] github feed failed, falling back to gitea:', err) @@ -102,7 +124,9 @@ async function checkWithFallback(): Promise { } useFeed('gitea') try { - await autoUpdater.checkForUpdates() + // Bounded as well: a still-stuck GitHub check is handed back to us here, and + // it must not leave the state at "checking" forever. + await withTimeout(autoUpdater.checkForUpdates(), CHECK_TIMEOUT_MS) } catch (err) { if (githubErr === undefined) throw err const giteaErr = err instanceof Error ? err.message : String(err) @@ -138,7 +162,12 @@ async function handleDownload(): Promise { async function directFetch(url: string): Promise { const s = session.fromPartition('openterminal-update-direct', { cache: false }) await s.setProxy({ mode: 'direct' }) - return s.fetch(url, { headers: { 'User-Agent': 'OpenTerminal' } }) + // Bounded: the changelog is on screen, so a stalled channel must fall through + // to the releases APIs instead of leaving the view spinning. + return s.fetch(url, { + headers: { 'User-Agent': 'OpenTerminal' }, + signal: AbortSignal.timeout(FETCH_TIMEOUT_MS) + }) } /** @@ -184,7 +213,10 @@ async function fetchChangelog(): Promise { } for (const url of [GITEA_RELEASES_API, GITHUB_RELEASES_API]) { try { - const resp = await net.fetch(url, { headers: { 'User-Agent': 'OpenTerminal' } }) + const resp = await net.fetch(url, { + headers: { 'User-Agent': 'OpenTerminal' }, + signal: AbortSignal.timeout(FETCH_TIMEOUT_MS) + }) if (!resp.ok) continue const data = (await resp.json()) as Array<{ tag_name?: string