diff --git a/src/main/commands.ts b/src/main/commands.ts index cea8590..18a56f7 100644 --- a/src/main/commands.ts +++ b/src/main/commands.ts @@ -20,7 +20,16 @@ import { app, shell } from 'electron' import { randomUUID } from 'crypto' -import { mkdirSync, readFileSync, writeFileSync, existsSync, realpathSync, statSync, copyFileSync } from 'fs' +import { + appendFileSync, + mkdirSync, + readFileSync, + writeFileSync, + existsSync, + realpathSync, + statSync, + copyFileSync +} from 'fs' import { appendFile } from 'fs/promises' import { basename, dirname, join, resolve, sep } from 'path' import type { CommandItem, SessionLogMeta } from '../shared/commands' @@ -83,6 +92,15 @@ function backupUnparseableCommands(file: string, err: unknown): void { } } +/** + * The single exit for an unusable commands.json — parse failure and wrong + * shape both end here, so neither can quietly skip the backup. + */ +function discardCommandsFile(file: string, err: unknown): CommandsFile { + backupUnparseableCommands(file, err) + return emptyFile() +} + export class CommandsStore { /** /commands.json */ private readonly file: string @@ -106,6 +124,17 @@ export class CommandsStore { private readonly logBuffers = new Map() /** In-flight drain loop per log file; absent when the file is settled. */ private readonly logDrains = new Map>() + /** + * Log files with an appendFile actually in flight. logStop reads this to tell + * whether a synchronous final flush can race an issued write (see logStop). + */ + private readonly logInFlight = new Set() + /** + * Files whose session stopped while a write was in flight: the drain loop + * flushes what is left synchronously instead of issuing another async round, + * which at quit would not land. Cleared when the loop settles. + */ + private readonly logFinalFlush = new Set() /** Plain-text transformer per actively-logged session (see logSanitizer). */ private readonly sanitizers = new Map() @@ -130,11 +159,17 @@ export class CommandsStore { library: Array.isArray(data.library) ? data.library : [] } } + // Valid JSON of the wrong shape (`[1,2,3]`, `{}`) is not "no commands": + // it is a file we cannot read, and the next write would rewrite it from + // an empty history. Same exit as the parse failure below. + return discardCommandsFile( + this.file, + new Error('commands.json shape mismatch: expected {history,library}') + ) } catch (err) { // missing -> start fresh; unreadable / corrupt -> keep the original first - backupUnparseableCommands(this.file, err) + return discardCommandsFile(this.file, err) } - return emptyFile() } private saveCommands(data: CommandsFile): void { @@ -412,15 +447,34 @@ export class CommandsStore { const chunk = this.logBuffers.get(file) if (chunk === undefined) break this.logBuffers.delete(file) + if (this.logFinalFlush.has(file)) { + // The session is gone, so this is the file's last flush and it has to + // be durable when this turn ends: at quit the process can be gone + // before a second async round lands. Reached only after the in-flight + // append above completed, so the order is still the write order. + try { + appendFileSync(file, chunk, 'utf8') + } catch { + // file may have been removed after stop -> ignore + } + continue + } + // Marked around the append itself: logStop's synchronous flush must not + // slip in between an issued write and its landing. + this.logInFlight.add(file) try { await appendFile(file, chunk, 'utf8') } catch { // file may have been removed after stop -> ignore } + this.logInFlight.delete(file) } })() run.finally(() => { if (this.logDrains.get(file) === run) this.logDrains.delete(file) + // The loop only stops once the buffer is empty, so nothing more can arrive + // for a stopped file: the flag must not outlive the drain. + this.logFinalFlush.delete(file) }) this.logDrains.set(file, run) return run @@ -432,10 +486,36 @@ export class CommandsStore { if (!meta) return const tail = this.sanitizers.get(sessionId)?.flush() ?? '' this.sanitizers.delete(sessionId) - if (tail) { - // Best effort: the trailing partial line belongs in the file too. - this.logBuffers.set(meta.file, (this.logBuffers.get(meta.file) ?? '') + tail) - this.drainLog(meta.file) + const pending = (this.logBuffers.get(meta.file) ?? '') + tail + if (pending) { + if (this.logInFlight.has(meta.file)) { + // An appendFile for this file is already issued and cannot be + // cancelled, so a synchronous write now would land *before* it and swap + // the last two pieces of the log. Put the tail back in the buffer + // instead and mark the file: the running loop re-reads the buffer after + // that append lands and flushes it synchronously (see drainLog), so the + // tail is durable and still in write order. + this.logBuffers.set(meta.file, pending) + this.logFinalFlush.add(meta.file) + this.drainLog(meta.file) + } else { + // No async write is in flight, so this one cannot race anything. It + // must be synchronous: the quit path (killAllPtys -> safeStopLog -> + // logStop) has no later sync point, and a fresh appendFile chain may + // never get to run. + // + // Bounded by construction: with nothing in flight the buffer is empty + // (the loop only stops looking once it is), so this is just the + // sanitizer tail — one partial line, plus at most one marker line. The + // sanitizer's only other buffer (a CSI sequence) is capped at 1KB and + // never reaches the output. One line's worth of text, once per stop. + this.logBuffers.delete(meta.file) + try { + appendFileSync(meta.file, pending, 'utf8') + } catch { + // file may have been removed after stop -> ignore + } + } } this.finishLog(meta) } diff --git a/src/main/connectionsStore.ts b/src/main/connectionsStore.ts index 1e2ac08..d5b4978 100644 --- a/src/main/connectionsStore.ts +++ b/src/main/connectionsStore.ts @@ -167,6 +167,15 @@ function backupUnparseableConnections(file: string, err: unknown): void { } } +/** + * The single exit for an unusable connections.json — parse failure and wrong + * shape both end here, so neither can quietly skip the backup. + */ +function discardConnectionsFile(file: string, err: unknown): StoredConnection[] { + backupUnparseableConnections(file, err) + return [] +} + export class ConnectionsStore { constructor(private readonly filePath: string) {} @@ -182,11 +191,17 @@ export class ConnectionsStore { typeof (x as StoredConnection).host === 'string' ) } + // Valid JSON of the wrong shape (`{}`) is not "no bookmarks": it is a file + // we cannot read, and the next write would rewrite it from an empty list. + // Same exit as the parse failure below. + return discardConnectionsFile( + this.filePath, + new Error('connections.json shape mismatch: expected an array') + ) } catch (err) { // missing -> start fresh; unreadable / corrupt -> keep the original first - backupUnparseableConnections(this.filePath, err) + return discardConnectionsFile(this.filePath, err) } - return [] } private save(list: StoredConnection[]): void { diff --git a/src/main/settingsStore.ts b/src/main/settingsStore.ts index 221e495..f157317 100644 --- a/src/main/settingsStore.ts +++ b/src/main/settingsStore.ts @@ -202,11 +202,23 @@ function refreshBuiltinRules(rules: HighlightRule[], warnings: Warnings): Highli return [...refreshed, ...missing.map((rule) => ({ ...rule }))].sort((a, b) => a.priority - b.priority) } +/** + * Fresh copies of the preset rules for a caller that may mutate what it gets + * back. Handing out the module-level array (or its objects) would let one + * store's edit leak into every later load in the process. + */ +function copyDefaultRules(): HighlightRule[] { + return DEFAULT_HIGHLIGHT_RULES.map((rule) => ({ ...rule })) +} + function sanitizeRules(value: unknown, warnings: Warnings): HighlightRule[] { // An explicit empty array is a valid choice ("no highlighting"); only // malformed data falls back to the built-in rules. Returning the defaults for // [] made deleting the last rule look like it silently failed. - if (!Array.isArray(value)) return DEFAULT_HIGHLIGHT_RULES + if (!Array.isArray(value)) { + warnings.push('highlightRules: not an array — built-in rules restored') + return copyDefaultRules() + } const rules: HighlightRule[] = [] value.forEach((entry, index) => { const rule = coerceRule(entry, index, warnings) @@ -457,22 +469,47 @@ function backupUnparseableSettings(err: unknown): void { } } +/** + * The defaults loadSettings falls back to. Fresh objects every call so a caller + * mutating what it got back cannot poison the module-level presets. + */ +function defaultSettings(): AppSettings { + return { + terminal: { ...DEFAULT_SETTINGS.terminal }, + customThemes: [...DEFAULT_SETTINGS.customThemes], + highlightRules: copyDefaultRules(), + highlightProfiles: [], + system: { ...DEFAULT_SYSTEM }, + lock: { ...DEFAULT_SETTINGS.lock } + } +} + +/** + * The single exit for an unusable settings.json — parse failure and wrong shape + * both end here, so neither can quietly skip the backup. + */ +function discardUnparseableSettings(err: unknown): AppSettings { + backupUnparseableSettings(err) + return defaultSettings() +} + export function loadSettings(): AppSettings { try { const raw: unknown = JSON.parse(readFileSync(settingsPath(), 'utf8')) + if (raw === null || typeof raw !== 'object' || Array.isArray(raw)) { + // Valid JSON of the wrong shape (`[1,2,3]`, `"text"`, `null`) is not "no + // settings": it is a file we cannot read, and the next mutation would + // rewrite it from the defaults — taking the custom themes and highlight + // rules with it. Per-field repair below only applies to a real object. + return discardUnparseableSettings( + new Error('settings.json shape mismatch: expected an object') + ) + } const { settings, errors } = deepMerge(raw) reportWarnings(errors) return settings } catch (err) { - backupUnparseableSettings(err) - return { - terminal: { ...DEFAULT_SETTINGS.terminal }, - customThemes: [...DEFAULT_SETTINGS.customThemes], - highlightRules: DEFAULT_HIGHLIGHT_RULES.map((rule) => ({ ...rule })), - highlightProfiles: [], - system: { ...DEFAULT_SYSTEM }, - lock: { ...DEFAULT_SETTINGS.lock } - } + return discardUnparseableSettings(err) } } diff --git a/tests/commands-store.mjs b/tests/commands-store.mjs index 11fc9c4..d5921dc 100644 --- a/tests/commands-store.mjs +++ b/tests/commands-store.mjs @@ -236,6 +236,21 @@ const expected = 'partial-tail' ok(readFileSync(startB.file, 'utf8') === expected, 'burst writes + stop tail land in order') +// The stop tail is flushed *synchronously* when no async append is in flight: +// the quit path (killAllPtys → safeStopLog → logStop) has no later sync point, +// so a fresh appendFile chain may never run and the tail would be lost. The +// drain for the committed line below has long settled, so nothing can race it. +const sid3 = 'cccccccc-dddd-eeee-ffff-000000000000' +const startC = store.logStart(sid3) +store.logWrite(sid3, 'settled line\n') +await wait(100) +store.logWrite(sid3, 'no trailing newline') +store.logStop(sid3) +ok( + readFileSync(startC.file, 'utf8') === 'settled line\nno trailing newline', + 'the stop tail is on disk when logStop returns (sync flush when the drain is idle)' +) + // ---- 7. index.json path containment (hydrateIndex) ----------------------------- // index.json is data, not trust: a tampered `file` value must never turn // logWrite into an arbitrary-path append. Only entries that resolve inside @@ -366,6 +381,36 @@ ok(!existsSync(join(freshDir, 'commands.json.bak')), 'a missing file (ENOENT) is ok(existsSync(join(freshDir, 'commands.json')), 'and the first write lands normally') rmSync(freshDir, { recursive: true, force: true }) +// ---- 10. Valid JSON of the wrong shape is backed up too ------------------------- +// `[1,2,3]` parses fine, so the parse-catch never saw it: the file used to be +// treated exactly like a missing one and the next recordCommand replaced it with +// an empty history. A file that is not the store we wrote is unreadable, not +// empty — same backup + empty-state exit as the corrupt case above. +const shapeDir = mkdtempSync(join(tmpdir(), 'm5-cmd-shape-')) +const shapeFile = join(shapeDir, 'commands.json') +const shapeStore = new commandsMod.CommandsStore(shapeDir) +const wrongShape = '[1,2,3]' +writeFileSync(shapeFile, wrongShape, 'utf8') +ok(shapeStore.listHistory().length === 0, 'a wrong-shaped commands.json loads as an empty history') +const shapeBak = `${shapeFile}.bak` +ok(existsSync(shapeBak), 'a wrong-shaped commands.json is backed up to .bak') +ok(readFileSync(shapeBak, 'utf8') === wrongShape, '.bak holds the wrong-shaped original byte for byte') +writeSettings({ historyLimit: 50, historyEnabled: true }) +shapeStore.recordCommand('after-shape-mismatch') +const rewrittenShape = JSON.parse(readFileSync(shapeFile, 'utf8')) +ok( + Array.isArray(rewrittenShape.history) && + rewrittenShape.history.some((h) => h.command === 'after-shape-mismatch'), + 'the store recovers with a well-formed file after the backup' +) + +// An object without the expected keys is the same class of problem, and a second +// episode must not overwrite the first backup. +writeFileSync(shapeFile, '{"library":[]}', 'utf8') +shapeStore.listHistory() +ok(readFileSync(shapeBak, 'utf8') === wrongShape, 'an existing .bak is kept for a wrong shape too (earliest evidence wins)') +rmSync(shapeDir, { recursive: true, force: true }) + // All stores wrote into temp dirs; drop them so repeated runs do not litter. rmSync(userData, { recursive: true, force: true }) diff --git a/tests/connections-store.mjs b/tests/connections-store.mjs index aeb508b..0561312 100644 --- a/tests/connections-store.mjs +++ b/tests/connections-store.mjs @@ -205,6 +205,42 @@ ok(!existsSync(join(freshDir, 'connections.json.bak')), 'a missing file (ENOENT) ok(existsSync(join(freshDir, 'connections.json')), 'and the first write lands normally') rmSync(freshDir, { recursive: true, force: true }) +// ---- 7b. Valid JSON of the wrong shape is backed up too -------------------------- +// `{}` parses fine, so the parse-catch never saw it: the file used to be treated +// exactly like a missing one and the next write replaced every bookmark with an +// empty list. A file that is not the array we wrote is unreadable, not empty — +// same backup + empty-state exit as the corrupt case above. +const shapeDir = mkdtempSync(join(tmpdir(), 'm-conn-shape-')) +const shapeFile = join(shapeDir, 'connections.json') +const shapeStore = new mod.ConnectionsStore(shapeFile) +const wrongShape = '{"connections":[]}' +writeFileSync(shapeFile, wrongShape, 'utf8') +ok(shapeStore.listConnections().length === 0, 'a wrong-shaped connections.json loads as an empty list') +const shapeBak = `${shapeFile}.bak` +ok(existsSync(shapeBak), 'a wrong-shaped connections.json is backed up to .bak') +ok(readFileSync(shapeBak, 'utf8') === wrongShape, '.bak holds the wrong-shaped original byte for byte') +const shapeSaved = shapeStore.saveConnection({ + name: 'after-shape-mismatch', + host: 'h', + port: 22, + username: 'u', + auth: 'password', + askPasswordAtConnect: false, + askPassphraseAtConnect: false, + keepaliveIntervalSec: 0 +}) +const shapeList = JSON.parse(readFileSync(shapeFile, 'utf8')) +ok( + Array.isArray(shapeList) && shapeList.length === 1 && shapeList[0].id === shapeSaved.id, + 'the new bookmark is written normally after the backup' +) + +// A second wrong-shape episode must not overwrite the first backup. +writeFileSync(shapeFile, '{"nope":true}', 'utf8') +shapeStore.listConnections() +ok(readFileSync(shapeBak, 'utf8') === wrongShape, 'an existing .bak is kept (earliest evidence wins)') +rmSync(shapeDir, { recursive: true, force: true }) + // An unreadable path (here: a directory) must still load as empty and never // throw — the backup is best effort and may itself fail. const dirCase = mkdtempSync(join(tmpdir(), 'm-conn-dir-')) diff --git a/tests/settings-store.mjs b/tests/settings-store.mjs index 5a2582b..5c90033 100644 --- a/tests/settings-store.mjs +++ b/tests/settings-store.mjs @@ -94,6 +94,27 @@ console.log('[highlight rules]') const out = (await roundTrip({ highlightRules: [] })).highlightRules ok(out.length === 0, 'an explicitly empty rule list stays empty') } +{ + // A non-array highlightRules is a shape error, not "no rules": the built-in + // rules come back as copies (mutating a loaded list must not poison the + // module-level preset table) and the fallback is recorded in the warnings log. + writeFileSync( + join(userData, 'settings.json'), + JSON.stringify({ highlightRules: { nope: true } }), + 'utf8' + ) + const first = store.loadSettings().highlightRules + ok(first.length > 0, `a non-array highlightRules loads the built-in rules (got ${first.length})`) + const second = store.loadSettings().highlightRules + ok(first !== second, 'each load hands out its own array, not the shared preset list') + first.length = 0 + const afterMutation = store.loadSettings().highlightRules + ok(afterMutation.length > 0, 'emptying a loaded rule list does not affect the next load') + const log = existsSync(join(userData, 'settings-warnings.log')) + ? readFileSync(join(userData, 'settings-warnings.log'), 'utf8') + : '' + ok(log.includes('highlightRules: not an array'), 'the fallback is recorded in settings-warnings.log') +} // ---- 3. value bands --------------------------------------------------------- console.log('[value bands]')