From 4e5377f49ca7f3356704e64922e36feff434762d Mon Sep 17 00:00:00 2001 From: Bill Date: Sun, 27 Sep 2026 22:25:03 +0800 Subject: [PATCH] fix: hardening round from code review (4 P1 + 15 P2) P1: - settingsStore: back up an unparseable settings.json to .bak before falling back to defaults, so the next mutation can no longer silently wipe custom themes/highlight rules - ptyDispatcher: fan out per-session data/exit handlers (Set instead of a single slot) so SSH split panes stop stealing each other's stream - FilePanel: monotonic refresh token keeps stale listings from painting over a newer navigation; upload finish no longer yanks the panel back - settings.css: active settings-tab label derives from --chrome-fg so it stays visible on the shipped light themes P2 (main/renderer): - paste guard: a paste ending in a newline always confirms - Workspace: closing an SSH pane no longer seeds the local cwd with a remote path - tray: skip close-dialog continuation on a destroyed window - commands: close zombie 'in-progress' session logs at hydrate - zmodem: clear the stale offer timer before arming a new one - connectionsStore: coerce/validate renderer input before persisting - sftp: OperationError marker class keeps translated errors out of the transport-retry classifier - CommandsPanel: surface save failures inside the dialog - ConnectionSidebar: drop a tautological tooltip condition P2 (i18n/tooling/tests): - localize the 16 ANSI color labels and the highlight sample text (21 new keys across zh-CN/zh-TW/en/ja) - sync-changelog: keep ### subheadings, normalize CRLF notes - release.cjs: GitHub release reuse-by-tag (idempotent re-runs); fail loudly on a failed Gitea asset listing - commands-store test: absent historyEnabled now truly tests absence --- scripts/release.cjs | 56 ++++++++++++++----- scripts/sync-changelog.cjs | 13 +++-- src/main/commands.ts | 12 +++- src/main/connectionsStore.ts | 38 ++++++++++++- src/main/settingsStore.ts | 27 ++++++++- src/main/sftp.ts | 21 ++++++- src/main/tray.ts | 8 +++ src/main/zmodem.ts | 5 ++ src/renderer/src/commands/CommandsPanel.tsx | 27 ++++++--- src/renderer/src/commands/commands.css | 8 +++ .../src/connections/ConnectionSidebar.tsx | 2 +- src/renderer/src/settings/HighlightTab.tsx | 2 +- src/renderer/src/settings/settings.css | 8 ++- src/renderer/src/sftp/FilePanel.tsx | 25 +++++++-- src/renderer/src/terminal/TerminalView.tsx | 18 ++++-- src/renderer/src/terminal/ptyDispatcher.ts | 40 +++++++++---- src/renderer/src/theme/editor/ThemeEditor.tsx | 37 ++++++------ src/renderer/src/workspace/Workspace.tsx | 9 ++- src/shared/i18n/dicts/en/common.ts | 1 + src/shared/i18n/dicts/en/settings.ts | 17 ++++++ src/shared/i18n/dicts/ja/common.ts | 1 + src/shared/i18n/dicts/ja/settings.ts | 17 ++++++ src/shared/i18n/dicts/zh-CN/common.ts | 1 + src/shared/i18n/dicts/zh-CN/settings.ts | 17 ++++++ src/shared/i18n/dicts/zh-TW/common.ts | 1 + src/shared/i18n/dicts/zh-TW/settings.ts | 17 ++++++ tests/commands-store.mjs | 11 ++-- 27 files changed, 356 insertions(+), 83 deletions(-) diff --git a/scripts/release.cjs b/scripts/release.cjs index 5f1d54b..c2ed8d5 100644 --- a/scripts/release.cjs +++ b/scripts/release.cjs @@ -124,7 +124,12 @@ async function gitea() { } } const listed = await fetch(`${base}/releases/${rel.id}/assets`, { headers: auth }) - const attached = new Set(listed.ok ? (await listed.json()).map((a) => a.name) : []) + // A failed listing must not read as "nothing attached": that would re-POST + // every already-uploaded installer and abort the run on the duplicate-name + // rejection, before the channel update could run. Fail loudly instead — a + // re-run then resumes correctly. + if (!listed.ok) throw new Error(`Gitea asset listing failed: HTTP ${listed.status}`) + const attached = new Set((await listed.json()).map((a) => a.name)) for (const f of files) { const name = path.basename(f) if (attached.has(name)) { console.log(` asset ${name}: already attached, skipped`); continue } @@ -237,20 +242,41 @@ async function giteaChannel() { async function github() { console.log('=== GitHub release ===') - const resp = await fetch('https://api.github.com/repos/billowliu2/OpenTerminal/releases', { - method: 'POST', - headers: { - Authorization: `Bearer ${env.GH_TOKEN}`, - 'Content-Type': 'application/json', - 'User-Agent': 'OpenTerminal', - Accept: 'application/vnd.github+json' - }, - body: JSON.stringify({ tag_name: `v${V}`, name: `OpenTerminal v${V}`, body: notes, draft: false, prerelease: false }), - ...(proxyAgent ? { dispatcher: proxyAgent } : {}) - }) - if (!resp.ok) { throw new Error(`GitHub release create failed: ${resp.status} ${(await resp.text()).slice(0, 300)}`) } - const rel = await resp.json() - console.log('release created, id =', rel.id) + const api = 'https://api.github.com/repos/billowliu2/OpenTerminal' + const ghHeaders = { + Authorization: `Bearer ${env.GH_TOKEN}`, + 'Content-Type': 'application/json', + 'User-Agent': 'OpenTerminal', + Accept: 'application/vnd.github+json' + } + const gh = (url, init) => + fetch(url, { ...init, headers: ghHeaders, ...(proxyAgent ? { dispatcher: proxyAgent } : {}) }) + // Idempotent like gitea(): a re-run after a partial publish must reuse the + // release it created last time instead of dying on 422 already_exists and + // leaving GitHub permanently half-uploaded until someone deletes it by hand. + const byTag = (): Promise => gh(`${api}/releases/tags/v${V}`) + let resp = await byTag() + let rel + if (resp.ok) { + rel = await resp.json() + console.log('release already exists, reusing id =', rel.id) + } else { + resp = await gh(`${api}/releases`, { + method: 'POST', + body: JSON.stringify({ tag_name: `v${V}`, name: `OpenTerminal v${V}`, body: notes, draft: false, prerelease: false }) + }) + if (resp.ok) { + rel = await resp.json() + console.log('release created, id =', rel.id) + } else { + const detail = (await resp.text()).slice(0, 300) + // 422 already_exists = the tag carries a release from an earlier partial run. + const again = resp.status === 422 ? await byTag() : undefined + if (!again || !again.ok) throw new Error(`GitHub release create failed: ${resp.status} ${detail}`) + rel = await again.json() + console.log('release already exists, reusing id =', rel.id) + } + } const up = `https://uploads.github.com/repos/billowliu2/OpenTerminal/releases/${rel.id}/assets` for (const f of [...files, path.join(R, `OpenTerminal-${V}-setup.exe.blockmap`), path.join(R, 'latest.yml')]) { await upload(up, f, env.GH_TOKEN, 'Bearer', true) diff --git a/scripts/sync-changelog.cjs b/scripts/sync-changelog.cjs index a635e58..87aa5de 100644 --- a/scripts/sync-changelog.cjs +++ b/scripts/sync-changelog.cjs @@ -33,10 +33,15 @@ const sync = ({ notes, changelog, header, required }) => { const body = fs .readFileSync(notesPath, 'utf8') - // Drop the notes' own title / `## vX` heading: the section heading is built - // here so every changelog stays uniform (`## vX.Y.Z - date`). - .replace(/^#.*\n/gm, '') - .replace(new RegExp(`^## v${pkgVersion}.*\\n`), '') + // Normalize newlines first: a CRLF notes file would otherwise keep its `\r` + // in every line, and the join below re-adds the changelog's ending on top — + // every line of the merged section rendering as `\r\r\n` (doubled blanks). + .replace(/\r\n/g, '\n') + // Drop only the notes' own title and the `## v` heading: the + // section heading is built here so every changelog stays uniform + // (`## vX.Y.Z - date`). Stripping every `#`-line (the old behaviour) also + // removed `###` subheadings, merging 新功能/修复 groups into one flat list. + .replace(new RegExp(`^# .*\\n|^## v${pkgVersion}.*\\n`, 'gm'), '') .replace(/^\s+/, '') .trimEnd() diff --git a/src/main/commands.ts b/src/main/commands.ts index 5fceb27..7b95c61 100644 --- a/src/main/commands.ts +++ b/src/main/commands.ts @@ -229,6 +229,7 @@ export class CommandsStore { if (!existsSync(this.indexFile)) return const raw: unknown = JSON.parse(readFileSync(this.indexFile, 'utf8')) if (!Array.isArray(raw)) return + let closed = false for (const x of raw) { if ( x && @@ -239,10 +240,19 @@ export class CommandsStore { // index.json is data, not trust: a tampered or hand-edited `file` // must never turn logWrite into an arbitrary-path append. if (!this.isLoggableFile(meta.file)) continue + // A log still "in progress" here was interrupted by a crash or a + // force-kill: its session id belonged to the previous run, so no + // logStop will ever match it and its sanitizer is gone (logWrite + // would be a permanent no-op). Close it instead of reviving it as + // an active entry nothing can ever end. + if (meta.endedAt === undefined) { + meta.endedAt = Date.now() + closed = true + } this.metasByFile.set(meta.file, meta) - if (meta.endedAt === undefined) this.activeBySession.set(meta.sessionId, meta) } } + if (closed) this.persistIndex() } catch { // index missing / corrupt -> rebuild on next write } diff --git a/src/main/connectionsStore.ts b/src/main/connectionsStore.ts index ec0c96d..fbf8959 100644 --- a/src/main/connectionsStore.ts +++ b/src/main/connectionsStore.ts @@ -20,6 +20,41 @@ import { writeJson } from './store' const ENC_SUFFIX = '_enc' const PLAIN_PREFIX = 'plain:' const SECRET_KEYS = ['password', 'keyContent', 'passphrase'] as const +const AUTH_METHODS: ReadonlySet = new Set(['password', 'privateKey', 'agent']) + +/** + * Runtime coercion for renderer-supplied connection fields. saveConnection + * copies input values verbatim, and they flow straight into ssh2's + * ConnectConfig (port, keepalive) — a malformed save or a renderer regression + * must not persist `port: "22"` or a NaN-bound keepalive. Repair over drop, + * like every other store: strings stay strings, numbers are range-checked, + * unknown auth falls back to password, optional fields collapse to undefined. + */ +function coerceConnectionInput(input: SshConnectionInput): SshConnectionInput { + const out = { ...input } as Record + out.name = typeof input.name === 'string' ? input.name : '' + out.group = typeof input.group === 'string' && input.group !== '' ? input.group : undefined + out.host = typeof input.host === 'string' ? input.host.trim() : '' + out.username = typeof input.username === 'string' ? input.username : '' + const port = Number(input.port) + out.port = Number.isInteger(port) && port >= 1 && port <= 65535 ? port : 22 + out.auth = AUTH_METHODS.has(input.auth as string) ? input.auth : 'password' + out.askPasswordAtConnect = input.askPasswordAtConnect === true + out.askPassphraseAtConnect = input.askPassphraseAtConnect === true + out.keyPath = + typeof input.keyPath === 'string' && input.keyPath !== '' ? input.keyPath : undefined + const keepalive = Number(input.keepaliveIntervalSec) + out.keepaliveIntervalSec = + Number.isFinite(keepalive) && keepalive >= 0 ? Math.round(keepalive) : 0 + out.highlightProfileId = + typeof input.highlightProfileId === 'string' && input.highlightProfileId !== '' + ? input.highlightProfileId + : undefined + for (const key of SECRET_KEYS) { + if (out[key] !== undefined && typeof out[key] !== 'string') out[key] = undefined + } + return out as unknown as SshConnectionInput +} /** Internal persisted record: public fields + encrypted secret fields. */ interface StoredConnection { @@ -150,7 +185,8 @@ export class ConnectionsStore { return stored ? decrypt(stored[encField] as string | undefined) : undefined } - saveConnection(input: SshConnectionInput): SshConnection { + saveConnection(rawInput: SshConnectionInput): SshConnection { + const input = coerceConnectionInput(rawInput) const list = this.load() let stored: StoredConnection | undefined diff --git a/src/main/settingsStore.ts b/src/main/settingsStore.ts index 9527e26..221e495 100644 --- a/src/main/settingsStore.ts +++ b/src/main/settingsStore.ts @@ -1,5 +1,5 @@ import { app, ipcMain, powerSaveBlocker } from 'electron' -import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'fs' +import { copyFileSync, existsSync, mkdirSync, readFileSync, writeFileSync } from 'fs' import { join } from 'path' import { Ipc } from '../shared/ipc' import { @@ -435,13 +435,36 @@ function reportWarnings(warnings: string[]): void { } } +/** + * A file that exists but cannot be parsed must never be silently destroyed: + * loadSettings falls back to defaults, and the next mutation (any settings save, + * even the tray persisting its close action) would rewrite settings.json from + * those defaults — taking the user's custom themes and highlight rules with it. + * Keep a one-time copy the user can recover from before that can happen. + */ +function backupUnparseableSettings(err: unknown): void { + try { + const file = settingsPath() + if (!existsSync(file)) return + const bak = `${file}.bak` + if (existsSync(bak)) return // keep the first backup; later loads must not clobber it + copyFileSync(file, bak) + reportWarnings([ + `settings.json unparseable (${err instanceof Error ? err.message : String(err)}) — original kept at settings.json.bak` + ]) + } catch { + // best effort: the backup must never break settings loading + } +} + export function loadSettings(): AppSettings { try { const raw: unknown = JSON.parse(readFileSync(settingsPath(), 'utf8')) const { settings, errors } = deepMerge(raw) reportWarnings(errors) return settings - } catch { + } catch (err) { + backupUnparseableSettings(err) return { terminal: { ...DEFAULT_SETTINGS.terminal }, customThemes: [...DEFAULT_SETTINGS.customThemes], diff --git a/src/main/sftp.ts b/src/main/sftp.ts index 071bcde..1144176 100644 --- a/src/main/sftp.ts +++ b/src/main/sftp.ts @@ -65,6 +65,14 @@ const sftpCache = new Map() */ class SessionGoneError extends Error {} +/** + * Our own "operation failed for a non-transport reason" error (server-side + * failure summary, user cancellation). isTransportError rejects these outright: + * their messages are translated and can embed remote paths/output, so text + * matching would make the retry decision locale- and filename-dependent. + */ +class OperationError extends Error {} + async function sftpOf(sessionId: string): Promise { const cached = sftpCache.get(sessionId) if (cached) return cached @@ -100,9 +108,16 @@ function evictSftp(sessionId: string): void { * already exists) mean the CHANNEL IS ALIVE — retrying them would just open * 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). */ function isTransportError(err: unknown): boolean { if (err instanceof SessionGoneError) 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) } @@ -211,7 +226,7 @@ export function deleteRemote(sessionId: string, paths: string[]): Promise } } if (failures.length > 0) { - throw new Error(t('main.sftp.deleteFailed', { detail: failures.join('; ') })) + throw new OperationError(t('main.sftp.deleteFailed', { detail: failures.join('; ') })) } }) } @@ -329,7 +344,7 @@ export function uploadRemote( let pos = 0 let lastEmit = 0 for (;;) { - if (transfer.cancelled) throw new Error(t('main.sftp.cancelled')) + if (transfer.cancelled) throw new OperationError(t('main.sftp.cancelled')) if (firstError) throw firstError while (inflight.size >= UPLOAD_WINDOW) { await Promise.race(inflight) @@ -420,7 +435,7 @@ export function downloadRemote( let lastEmit = 0 const buf = Buffer.alloc(CHUNK) for (;;) { - if (transfer.cancelled) throw new Error(t('main.sftp.cancelled')) + if (transfer.cancelled) throw new OperationError(t('main.sftp.cancelled')) const { bytesRead } = await p<{ bytesRead: number; buffer: Buffer }>(cb => sftp.read(handle, buf, 0, CHUNK, pos, cb) ) diff --git a/src/main/tray.ts b/src/main/tray.ts index 7cbd47d..fec1517 100644 --- a/src/main/tray.ts +++ b/src/main/tray.ts @@ -87,6 +87,9 @@ export function initTray(showOrCreate: () => void): void { } function hideToTray(win: BrowserWindow): void { + // A quit started while the close dialog was pending destroys the window + // before its resolution runs; touching a destroyed window throws. + if (win.isDestroyed()) return win.hide() if (process.platform === 'win32' && tray && !balloonShown) { balloonShown = true @@ -126,6 +129,11 @@ export async function onMainWindowClose(win: BrowserWindow, e: Electron.Event, s checkboxChecked: false, noLink: true }) + // The dialog resolves when the window is destroyed underneath it (tray exit + // or OS shutdown called app.quit() while it was open): the quit is already + // underway, so neither branch may run — and must not persist a choice the + // user never made. + if (win.isDestroyed()) return if (response === 0) { if (checkboxChecked) persistCloseAction('tray') hideToTray(win) diff --git a/src/main/zmodem.ts b/src/main/zmodem.ts index 6343937..74acfc3 100644 --- a/src/main/zmodem.ts +++ b/src/main/zmodem.ts @@ -342,6 +342,11 @@ export function attachZmodem(sessionId: string, deps: ZmodemDeps): void { } catch { // never crash the event loop } + // Clear any previous offer timer first: a re-detect while an earlier + // offer is still pending overwrites engine.offerTimer, and the orphaned + // timer would fire at its original deadline and abort a legitimate + // pending offer ahead of its own timeout. + if (engine.offerTimer) clearTimeout(engine.offerTimer) engine.offerTimer = setTimeout(() => { if (engine.detection && !engine.confirmed) { abortWithoutSession(engine, t('main.zmodem.offerTimedOut')) diff --git a/src/renderer/src/commands/CommandsPanel.tsx b/src/renderer/src/commands/CommandsPanel.tsx index 166d1b6..f61c292 100644 --- a/src/renderer/src/commands/CommandsPanel.tsx +++ b/src/renderer/src/commands/CommandsPanel.tsx @@ -31,6 +31,8 @@ export function CommandsPanel({ onRun }: CommandsPanelProps): React.JSX.Element const [name, setName] = useState('') const [command, setCommand] = useState('') const [note, setNote] = useState('') + /** Save failure surfaced inside the dialog instead of an unhandled rejection. */ + const [saveError, setSaveError] = useState('') /** M6.1: current workspace group — the command panel's 发送目标 belongs to it. */ const workspaceMode = useWorkspaceModeStore((s) => s.mode) @@ -68,20 +70,28 @@ export function CommandsPanel({ onRun }: CommandsPanelProps): React.JSX.Element setName('') setCommand('') setNote('') + setSaveError('') setAddOpen(true) }, []) const handleSave = useCallback(async (): Promise => { const trimmed = command.trim() if (!trimmed) return - await window.api.saveLibraryItem({ - id: window.crypto.randomUUID(), - name: name.trim() || undefined, - command: trimmed, - note: note.trim() || undefined, - createdAt: Date.now(), - lastUsedAt: Date.now() - }) + try { + await window.api.saveLibraryItem({ + id: window.crypto.randomUUID(), + name: name.trim() || undefined, + command: trimmed, + note: note.trim() || undefined, + createdAt: Date.now(), + lastUsedAt: Date.now() + }) + } catch (err) { + // Keep the dialog open with the typed values; a silent no-op would look + // like the save button doing nothing. + setSaveError((err as Error)?.message || t('common.saveFailed')) + return + } setAddOpen(false) refresh() }, [name, command, note, refresh]) @@ -182,6 +192,7 @@ export function CommandsPanel({ onRun }: CommandsPanelProps): React.JSX.Element className="commands-modal" >
+ {saveError &&
{saveError}
}