diff --git a/.gitignore b/.gitignore index 86950c0..906aa19 100644 --- a/.gitignore +++ b/.gitignore @@ -24,6 +24,7 @@ research/ .sftp-dl-* tests/.commands-store.cjs +tests/.connections-store.cjs tests/.known-hosts.cjs tests/.lock-store.cjs tests/.lock-controller.cjs diff --git a/package.json b/package.json index 9a944d3..a5f1c30 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ "preview": "electron-vite preview", "typecheck": "tsc --noEmit -p tsconfig.node.json && tsc --noEmit -p tsconfig.web.json", "pretest": "npm run typecheck", - "test": "node tests/build-bundles.cjs && node tests/ssh-loopback.mjs && node tests/commands-store.mjs && node tests/settings-store.mjs && node tests/lock-store.mjs && node tests/lock-controller.mjs && node tests/lock-shortcuts.mjs && node tests/.hl-split-smoke.cjs && node tests/.hl-rules.cjs && node tests/zmodem-e2e.mjs && node tests/ssh-session-e2e.mjs && node tests/sysinfo-e2e.mjs", + "test": "node tests/build-bundles.cjs && node tests/ssh-loopback.mjs && node tests/commands-store.mjs && node tests/connections-store.mjs && node tests/settings-store.mjs && node tests/lock-store.mjs && node tests/lock-controller.mjs && node tests/lock-shortcuts.mjs && node tests/.hl-split-smoke.cjs && node tests/.hl-rules.cjs && node tests/zmodem-e2e.mjs && node tests/ssh-session-e2e.mjs && node tests/sysinfo-e2e.mjs", "predist": "npm test && npm install --package-lock-only", "dist": "electron-vite build && electron-builder --win msi nsis", "dist:dir": "electron-vite build && electron-builder --win --dir" diff --git a/src/main/commands.ts b/src/main/commands.ts index 7b95c61..cea8590 100644 --- a/src/main/commands.ts +++ b/src/main/commands.ts @@ -20,7 +20,7 @@ import { app, shell } from 'electron' import { randomUUID } from 'crypto' -import { mkdirSync, readFileSync, writeFileSync, existsSync, realpathSync, statSync } from 'fs' +import { 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' @@ -60,6 +60,29 @@ function stamp(d = new Date()): string { ) } +/** + * A commands.json that exists but cannot be read or parsed must never be + * silently destroyed: loadCommands falls back to an empty file, and the next + * write (recording one command, or saving a library item) would rewrite the + * file from that empty state — taking the whole command history *and* the + * library with it. Keep a one-time copy the user can recover from before that + * can happen. + */ +function backupUnparseableCommands(file: string, err: unknown): void { + try { + // ENOENT (first run) is the normal empty case: nothing to lose, stay quiet. + 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) + console.warn( + `[commands] ${file} unparseable (${err instanceof Error ? err.message : String(err)}) — original kept at ${bak}` + ) + } catch { + // best effort: the backup must never break loading + } +} + export class CommandsStore { /** /commands.json */ private readonly file: string @@ -107,8 +130,9 @@ export class CommandsStore { library: Array.isArray(data.library) ? data.library : [] } } - } catch { - // missing / corrupted file -> start fresh + } catch (err) { + // missing -> start fresh; unreadable / corrupt -> keep the original first + backupUnparseableCommands(this.file, err) } return emptyFile() } diff --git a/src/main/connectionsStore.ts b/src/main/connectionsStore.ts index fbf8959..1e2ac08 100644 --- a/src/main/connectionsStore.ts +++ b/src/main/connectionsStore.ts @@ -13,7 +13,7 @@ import { safeStorage } from 'electron' import { randomUUID } from 'crypto' -import { readFileSync } from 'fs' +import { copyFileSync, existsSync, readFileSync } from 'fs' import type { SshAuthMethod, SshConnection, SshConnectionInput } from '../shared/connections' import { writeJson } from './store' @@ -144,6 +144,29 @@ function toPublic(stored: StoredConnection): SshConnection { } } +/** + * A connections.json that exists but cannot be read or parsed must never be + * silently destroyed: load() falls back to an empty list, and the next write + * (any bookmark edit, or just a `touch()` after connecting) would rewrite the + * file from that empty list — taking every saved host and its `*_enc` + * credentials with it. Keep a one-time copy the user can recover from before + * that can happen. + */ +function backupUnparseableConnections(file: string, err: unknown): void { + try { + // ENOENT (first run) is the normal empty case: nothing to lose, stay quiet. + 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) + console.warn( + `[connections] ${file} unparseable (${err instanceof Error ? err.message : String(err)}) — original kept at ${bak}` + ) + } catch { + // best effort: the backup must never break loading + } +} + export class ConnectionsStore { constructor(private readonly filePath: string) {} @@ -159,8 +182,9 @@ export class ConnectionsStore { typeof (x as StoredConnection).host === 'string' ) } - } catch { - // missing / corrupted file -> start fresh + } catch (err) { + // missing -> start fresh; unreadable / corrupt -> keep the original first + backupUnparseableConnections(this.filePath, err) } return [] } diff --git a/tests/build-bundles.cjs b/tests/build-bundles.cjs index 8331523..cd40381 100644 --- a/tests/build-bundles.cjs +++ b/tests/build-bundles.cjs @@ -19,6 +19,10 @@ const BUNDLES = [ // ESM (`.mjs`): tests/sftp-*.mjs load it with `await import()`. { entry: 'src/main/sftp.ts', out: 'tests/.sftp-svc.mjs', format: 'esm', external: ['ssh2'] }, { entry: 'src/main/commands.ts', out: 'tests/.commands-store.cjs' }, + // SSH bookmarks store: CRUD, the public/secret split, corrupt-file backup. + // Its electron surface is `safeStorage` (already in the stub, unavailable -> + // the store's `plain:` dev fallback). + { entry: 'src/main/connectionsStore.ts', out: 'tests/.connections-store.cjs' }, // Known-hosts store: TOFU / changed / unreadable fail-closed behavior. { entry: 'src/main/knownHosts.ts', out: 'tests/.known-hosts.cjs' }, { entry: 'src/main/settingsStore.ts', out: 'tests/.settings-store.cjs' }, diff --git a/tests/commands-store.mjs b/tests/commands-store.mjs index d6e0b25..11fc9c4 100644 --- a/tests/commands-store.mjs +++ b/tests/commands-store.mjs @@ -334,6 +334,38 @@ kh.accept('h1', 22, keyB, fpB) ok(kh.check('h1', 22, keyB).status === 'match', 'knownHosts: accept works again once the store is readable') rmSync(khDir, { recursive: true, force: true }) +// ---- 9. Corrupt commands.json is backed up, never silently replaced ------------- +// A file that exists but cannot be parsed used to be treated exactly like a +// missing one: the next recordCommand wrote a fresh file from an empty history, +// destroying the whole history + library in one write. The original must be +// copied aside first (ENOENT is still silent — there is nothing to lose). +const corrupt = '{"history":[{"id":"keep-me","command":"ps aux"}' +writeFileSync(join(userData, 'commands.json'), corrupt, 'utf8') +writeSettings({ historyLimit: 50, historyEnabled: true }) +store.recordCommand('after-corruption') +const bakFile = join(userData, 'commands.json.bak') +ok(existsSync(bakFile), 'corrupt commands.json is backed up to .bak') +ok(readFileSync(bakFile, 'utf8') === corrupt, '.bak holds the corrupt original byte for byte') +const rewritten = JSON.parse(readFileSync(join(userData, 'commands.json'), 'utf8')) +ok( + Array.isArray(rewritten.history) && rewritten.history.some((h) => h.command === 'after-corruption'), + 'the new command is written normally after the backup' +) + +// A second corrupt episode must not overwrite the first backup: the earliest +// copy is the one that still holds the user's real data. +writeFileSync(join(userData, 'commands.json'), 'not json at all', 'utf8') +store.recordCommand('second-corruption') +ok(readFileSync(bakFile, 'utf8') === corrupt, 'an existing .bak is kept (earliest evidence wins)') + +// A store whose file simply does not exist yet (first run) backs up nothing. +const freshDir = mkdtempSync(join(tmpdir(), 'm5-cmd-fresh-')) +const store4 = new commandsMod.CommandsStore(freshDir) +store4.recordCommand('first-ever') +ok(!existsSync(join(freshDir, 'commands.json.bak')), 'a missing file (ENOENT) is not backed up') +ok(existsSync(join(freshDir, 'commands.json')), 'and the first write lands normally') +rmSync(freshDir, { 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 new file mode 100644 index 0000000..aeb508b --- /dev/null +++ b/tests/connections-store.mjs @@ -0,0 +1,228 @@ +/** + * SSH connections-store self-test (connections-store.mjs). + * + * Bundles src/main/connectionsStore.ts for plain Node with the in-memory + * electron stub, then verifies against a temp userData dir: bookmark CRUD, + * input coercion, the public/secret split (no `*_enc` value ever leaves the + * store, secrets come back only through getSecret) and the corrupt-file + * backup guard. + * + * The stub reports safeStorage as unavailable, so the store takes its + * documented dev fallback and persists secrets as `plain:`; the tests + * below assert on that prefix rather than on OS-encrypted ciphertext. + * + * Build: npx esbuild src/main/connectionsStore.ts --bundle --platform=node \ + * --format=cjs --outfile=tests/.connections-store.cjs \ + * --alias:electron=./tests/electron-stub.cjs + * Run: node tests/connections-store.mjs (must exit 0) + */ +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'fs' +import { join } from 'path' +import { tmpdir } from 'os' +import { createRequire } from 'module' +const require_ = createRequire(import.meta.url) + +const fail = (msg) => { + console.error(`FAIL: ${msg}`) + process.exit(1) +} +const ok = (cond, msg) => { + if (!cond) fail(msg) + console.log(` ok: ${msg}`) +} + +// ---- 1. Bundle the store once (assumes tests/.connections-store.cjs exists) ----- +const userData = mkdtempSync(join(tmpdir(), 'm-conn-')) +process.env.OT_STUB_USERDATA = userData +let mod +try { + mod = require_('./.connections-store.cjs') +} catch { + fail('bundle not found — run: node tests/build-bundles.cjs (or the esbuild line in the header)') +} + +// ---- 2. Store over a temp file ------------------------------------------------- +const file = join(userData, 'connections.json') +ok( + mod.defaultConnectionsPath(userData) === `${userData}/connections.json`, + 'defaultConnectionsPath is /connections.json' +) +const store = new mod.ConnectionsStore(file) +const rawList = () => JSON.parse(readFileSync(file, 'utf8')) + +ok(store.listConnections().length === 0, 'missing file -> empty list, no throw') + +// ---- 3. Create + the public/secret split --------------------------------------- +const a = store.saveConnection({ + name: 'web', + host: '10.0.0.1', + port: 22, + username: 'root', + auth: 'password', + askPasswordAtConnect: false, + askPassphraseAtConnect: false, + keepaliveIntervalSec: 30, + password: 's3cret', + highlightProfileId: 'prof-1' +}) +ok(typeof a.id === 'string' && a.id.length > 0, 'saveConnection assigns an id + createdAt') +ok(a.savedAuth.hasPassword === true, 'savedAuth.hasPassword reflects the stored secret') +ok(!('password' in a) && !('password_enc' in a), 'the returned record carries no secret field') +ok(a.highlightProfileId === 'prof-1', 'highlightProfileId round-trips') + +const persistedA = rawList().find((c) => c.id === a.id) +ok(persistedA.password_enc.startsWith('plain:'), 'secret is persisted via the safeStorage fallback marker') +ok(!('password' in persistedA), 'plaintext secret is never written to disk') +ok(store.getSecret(a, 'password') === 's3cret', 'getSecret decrypts the stored password') +ok(store.getSecret(a, 'keyContent') === undefined, 'getSecret of an absent secret is undefined') + +// ---- 4. Update in place: omitted secrets survive, '' clears --------------------- +const updated = store.saveConnection({ + id: a.id, + name: 'web-2', + host: '10.0.0.2', + port: 2222, + username: 'admin', + auth: 'privateKey', + askPasswordAtConnect: false, + askPassphraseAtConnect: true, + keepaliveIntervalSec: 0, + keyContent: 'KEY' +}) +ok(store.listConnections().length === 1, 'update by id keeps the count at 1') +ok(updated.name === 'web-2' && updated.host === '10.0.0.2' && updated.port === 2222, 'public fields updated') +ok(updated.createdAt === a.createdAt, 'update preserves createdAt') +ok(updated.savedAuth.hasPassword === true, 'omitting a secret keeps the previously stored one') +ok(store.getSecret(updated, 'password') === 's3cret', 'and that secret still decrypts') +ok(store.getSecret(updated, 'keyContent') === 'KEY', 'newly supplied secret is stored') +ok(updated.highlightProfileId === undefined, 'an omitted highlightProfileId clears the binding') + +const cleared = store.saveConnection({ + id: a.id, + name: 'web-2', + host: '10.0.0.2', + port: 2222, + username: 'admin', + auth: 'password', + askPasswordAtConnect: true, + askPassphraseAtConnect: false, + keepaliveIntervalSec: 0, + password: '' +}) +ok(cleared.savedAuth.hasPassword === false, 'an empty secret string clears the stored value') +ok(store.getSecret(cleared, 'password') === undefined, 'and the cleared secret reads back undefined') + +// ---- 5. Malformed input is coerced, never persisted as-is ----------------------- +const coerced = store.saveConnection({ + name: 'bad', + host: ' 10.9.9.9 ', + port: '22', + username: 'u', + auth: 'nope', + askPasswordAtConnect: 'yes', + askPassphraseAtConnect: 1, + keepaliveIntervalSec: Number.NaN, + group: '', + keyPath: '', + highlightProfileId: '' +}) +ok(coerced.port === 22, 'a string port is coerced to a number') +ok(coerced.host === '10.9.9.9', 'host is trimmed') +ok(coerced.auth === 'password', 'unknown auth falls back to password') +ok(coerced.askPasswordAtConnect === false, 'a non-boolean ask flag collapses to false') +ok(coerced.keepaliveIntervalSec === 0, 'a NaN keepalive collapses to 0') +ok(coerced.group === undefined && coerced.keyPath === undefined, 'empty optional strings collapse to undefined') +ok(coerced.highlightProfileId === undefined, 'an empty highlightProfileId collapses to undefined') +const outOfRange = store.saveConnection({ + name: 'bad-port', + host: 'h', + port: 70000, + username: 'u', + auth: 'password', + askPasswordAtConnect: false, + askPassphraseAtConnect: false, + keepaliveIntervalSec: -5 +}) +ok(outOfRange.port === 22 && outOfRange.keepaliveIntervalSec === 0, 'out-of-range numbers fall back to defaults') + +// ---- 6. touch / delete --------------------------------------------------------- +store.touch(coerced.id, 1234567890) +const touched = store.listConnections().find((c) => c.id === coerced.id) +ok(touched.lastConnectedAt === 1234567890, 'touch persists lastConnectedAt') +const beforeTouch = rawList().length +store.touch('no-such-id') +ok(rawList().length === beforeTouch, 'touch of an unknown id is a no-op') + +store.deleteConnection(coerced.id) +ok(!store.listConnections().some((c) => c.id === coerced.id), 'deleteConnection removes the bookmark') +const afterDelete = rawList().length +store.deleteConnection('no-such-id') +ok(rawList().length === afterDelete, 'delete of an unknown id is a no-op') + +// ---- 7. Corrupt connections.json is backed up, never silently replaced ---------- +// A file that exists but cannot be parsed used to be treated exactly like a +// missing one: the next write (a bookmark edit, or just a touch() after +// connecting) rewrote the file from an empty list, destroying every saved host +// and its credentials. The original must be copied aside first. +const survivors = rawList() +const corrupt = `${JSON.stringify(survivors).slice(0, 40)}` // truncated JSON: parses nowhere +writeFileSync(file, corrupt, 'utf8') +store.saveConnection({ + name: 'after-corruption', + host: 'h', + port: 22, + username: 'u', + auth: 'password', + askPasswordAtConnect: false, + askPassphraseAtConnect: false, + keepaliveIntervalSec: 0 +}) +const bak = `${file}.bak` +ok(existsSync(bak), 'corrupt connections.json is backed up to .bak') +ok(readFileSync(bak, 'utf8') === corrupt, '.bak holds the corrupt original byte for byte') +const rewritten = rawList() +ok(Array.isArray(rewritten) && rewritten.length === 1 && rewritten[0].name === 'after-corruption', 'the new bookmark is written normally after the backup') + +// A second corrupt episode must not overwrite the first backup. +writeFileSync(file, 'not json at all', 'utf8') +store.listConnections() +ok(readFileSync(bak, 'utf8') === corrupt, 'an existing .bak is kept (earliest evidence wins)') + +// A store whose file does not exist yet (first run) backs up nothing. +const freshDir = mkdtempSync(join(tmpdir(), 'm-conn-fresh-')) +const fresh = new mod.ConnectionsStore(join(freshDir, 'connections.json')) +fresh.saveConnection({ + name: 'first', + host: 'h', + port: 22, + username: 'u', + auth: 'password', + askPasswordAtConnect: false, + askPassphraseAtConnect: false, + keepaliveIntervalSec: 0 +}) +ok(!existsSync(join(freshDir, 'connections.json.bak')), 'a missing file (ENOENT) is not backed up') +ok(existsSync(join(freshDir, 'connections.json')), 'and the first write lands normally') +rmSync(freshDir, { 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-')) +mkdirSync(join(dirCase, 'connections.json')) +const dirStore = new mod.ConnectionsStore(join(dirCase, 'connections.json')) +let listed +let threw = false +try { + listed = dirStore.listConnections() +} catch { + threw = true +} +ok(!threw && listed.length === 0, 'an unreadable file (EISDIR) loads as empty instead of throwing') +ok(!existsSync(join(dirCase, 'connections.json.bak')), 'and a backup that cannot be made leaves no litter') +rmSync(dirCase, { recursive: true, force: true }) + +// All stores wrote into temp dirs; drop them so repeated runs do not litter. +rmSync(userData, { recursive: true, force: true }) + +console.log('\n[connections] ALL CHECKS PASSED') +process.exit(0)