fix: hardening round from code review (4 P1 + 15 P2)
CI / typecheck + test + build (windows) (push) Canceled after 0s
CI / typecheck + test + build (windows) (push) Canceled after 0s
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
This commit is contained in:
1 parent
33efbe921e
commit
4e5377f49c
27 files changed
+356
-83
No files matched your search
+11
-1
@@ -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
|
||||
}
|
||||
|
||||
@@ -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<string> = 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<string, unknown>
|
||||
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
|
||||
|
||||
|
||||
@@ -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],
|
||||
|
||||
+18
-3
@@ -65,6 +65,14 @@ const sftpCache = new Map<string, SFTPWrapper>()
|
||||
*/
|
||||
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<SFTPWrapper> {
|
||||
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<void>
|
||||
}
|
||||
}
|
||||
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)
|
||||
)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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'))
|
||||
|
||||
Reference in new issue
Block a user