From 58d54311d837efe87e73cc0093d8a6e68b147313 Mon Sep 17 00:00:00 2001 From: sudacode Date: Wed, 8 Jul 2026 23:51:46 -0700 Subject: [PATCH] fix(launcher): address code review findings in stats sync - merge-catalog: stop dropping anilist_id on every new anime insert (bun:sqlite .get() returns null, so the old !== undefined guard always fired); the guard was dead anyway since a colliding id is caught earlier - sync-command: throw instead of fail() inside runHostSync so the finally block runs and temp dirs holding snapshot data are cleaned up on failure - ssh: shell-quote the user-supplied --remote-cmd in the probe, and reject option-like (-prefixed) SSH hosts that ssh/scp would parse as flags - sync-db: close the remote handle if opening the local DB throws - sync-shared: treat EPERM from process.kill(pid, 0) as alive, not dead - cli-parser: trim the sync host before the mode-exclusivity check - tests: cover anilist_id preservation, anilist-based anime matching, and the SSH host/shell-quote guards --- launcher/commands/sync-command.ts | 18 ++++++-- launcher/config/cli-parser-builder.ts | 5 ++- launcher/sync/merge-catalog.ts | 12 ++---- launcher/sync/ssh.test.ts | 19 +++++++++ launcher/sync/ssh.ts | 25 ++++++++++-- launcher/sync/sync-db.test.ts | 59 +++++++++++++++++++++++++++ launcher/sync/sync-db.ts | 8 +++- launcher/sync/sync-shared.ts | 6 ++- 8 files changed, 131 insertions(+), 21 deletions(-) create mode 100644 launcher/sync/ssh.test.ts diff --git a/launcher/commands/sync-command.ts b/launcher/commands/sync-command.ts index fdb4967e..45cc9b16 100644 --- a/launcher/commands/sync-command.ts +++ b/launcher/commands/sync-command.ts @@ -9,7 +9,13 @@ import { formatMergeSummary, mergeSnapshotIntoDb, } from '../sync/sync-db.js'; -import { resolveRemoteSubminerCommand, runScp, runSsh, shellQuote } from '../sync/ssh.js'; +import { + assertSafeSshHost, + resolveRemoteSubminerCommand, + runScp, + runSsh, + shellQuote, +} from '../sync/ssh.js'; import { resolvePathMaybe } from '../util.js'; import type { LauncherCommandContext } from './context.js'; @@ -66,6 +72,7 @@ function cleanupRemote(host: string, remoteTmpDir: string): void { function runHostSync(context: LauncherCommandContext, dbPath: string): void { const { args } = context; const host = args.syncHost; + assertSafeSshHost(host); ensureTrackerQuiescent(context, dbPath); @@ -75,10 +82,13 @@ function runHostSync(context: LauncherCommandContext, dbPath: string): void { const localTmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'subminer-sync-')); let remoteTmpDir = ''; try { + // Signal failures by throwing (not fail(), which exits synchronously and + // would skip the finally cleanup, leaking temp dirs holding snapshot data). + // main().catch() reports the message the same way fail() would. const mktemp = runSsh(host, 'mktemp -d /tmp/subminer-sync.XXXXXX'); remoteTmpDir = mktemp.stdout.trim(); if (mktemp.status !== 0 || !remoteTmpDir.startsWith('/tmp/')) { - fail(`Could not create a temporary directory on ${host}.`); + throw new Error(`Could not create a temporary directory on ${host}.`); } const forceFlag = args.syncForce ? ' --force' : ''; @@ -94,7 +104,7 @@ function runHostSync(context: LauncherCommandContext, dbPath: string): void { `${remoteCmd} sync --snapshot ${shellQuote(remoteSnapshot)}${forceFlag}`, ); if (snapshotRun.status !== 0) { - fail(`Remote snapshot failed on ${host}.`); + throw new Error(`Remote snapshot failed on ${host}.`); } const pulledSnapshot = path.join(localTmpDir, 'remote.sqlite'); @@ -113,7 +123,7 @@ function runHostSync(context: LauncherCommandContext, dbPath: string): void { ); process.stdout.write(mergeRun.stdout); if (mergeRun.status !== 0) { - fail( + throw new Error( `Remote merge failed on ${host}. The local database was updated; re-run "subminer sync ${host}" once the remote issue is fixed.`, ); } diff --git a/launcher/config/cli-parser-builder.ts b/launcher/config/cli-parser-builder.ts index e8b62c91..f08f3987 100644 --- a/launcher/config/cli-parser-builder.ts +++ b/launcher/config/cli-parser-builder.ts @@ -316,7 +316,8 @@ export function parseCliPrograms( .option('--remote-cmd ', 'subminer command to run on the remote host') .option('-f, --force', 'Skip the running-app safety check') .option('--log-level ', 'Log level') - .action((host: string | undefined, options: Record) => { + .action((rawHost: string | undefined, options: Record) => { + const host = typeof rawHost === 'string' ? rawHost.trim() : ''; const snapshot = typeof options.snapshot === 'string' ? options.snapshot.trim() : ''; const merge = typeof options.merge === 'string' ? options.merge.trim() : ''; const modes = [Boolean(host), Boolean(snapshot), Boolean(merge)].filter(Boolean).length; @@ -327,7 +328,7 @@ export function parseCliPrograms( throw new Error('Sync host, --snapshot, and --merge cannot be combined.'); } syncTriggered = true; - syncHost = host?.trim() || null; + syncHost = host || null; syncSnapshotPath = snapshot || null; syncMergePath = merge || null; syncRemoteCmd = typeof options.remoteCmd === 'string' ? options.remoteCmd.trim() || null : null; diff --git a/launcher/sync/merge-catalog.ts b/launcher/sync/merge-catalog.ts index bd22c53a..56f22ad2 100644 --- a/launcher/sync/merge-catalog.ts +++ b/launcher/sync/merge-catalog.ts @@ -138,15 +138,9 @@ export function mergeAnime( ); continue; } - // anilist_id is UNIQUE; drop it if another local row (matched by a - // different title key) already claims it. - const anilistTaken = - row.anilist_id !== null && byAnilist.get(row.anilist_id) !== undefined - ? null - : row.anilist_id; - const values = ANIME_COPY_COLUMNS.map((column) => - column === 'anilist_id' ? anilistTaken : row[column], - ); + // No local row matched by anilist_id (checked first in `existing` above) + // or title key, so the remote anilist_id — if any — is free to insert as-is. + const values = ANIME_COPY_COLUMNS.map((column) => row[column]); map.set(remoteId, insertRow(local, 'imm_anime', ANIME_COPY_COLUMNS, values)); summary.animeAdded += 1; } diff --git a/launcher/sync/ssh.test.ts b/launcher/sync/ssh.test.ts new file mode 100644 index 00000000..44beb578 --- /dev/null +++ b/launcher/sync/ssh.test.ts @@ -0,0 +1,19 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { assertSafeSshHost, shellQuote } from './ssh.js'; + +test('assertSafeSshHost rejects option-like hosts', () => { + assert.throws(() => assertSafeSshHost('-oProxyCommand=touch pwned'), /looks like an option/); + assert.throws(() => assertSafeSshHost('-lroot'), /looks like an option/); +}); + +test('assertSafeSshHost accepts normal destinations', () => { + assert.doesNotThrow(() => assertSafeSshHost('macbook')); + assert.doesNotThrow(() => assertSafeSshHost('user@192.168.1.20')); + assert.doesNotThrow(() => assertSafeSshHost('ssh-alias')); +}); + +test('shellQuote escapes single quotes and wraps in quotes', () => { + assert.equal(shellQuote('subminer'), `'subminer'`); + assert.equal(shellQuote(`a'; rm -rf ~; '`), `'a'\\''; rm -rf ~; '\\'''`); +}); diff --git a/launcher/sync/ssh.ts b/launcher/sync/ssh.ts index c7435ec3..ca7df226 100644 --- a/launcher/sync/ssh.ts +++ b/launcher/sync/ssh.ts @@ -5,12 +5,24 @@ export interface RemoteRunResult { stdout: string; } +/** + * ssh/scp have no `--` terminator for the destination, so a host that starts + * with `-` (e.g. `-oProxyCommand=...`) is parsed as an option. Reject those + * before spawning. + */ +export function assertSafeSshHost(host: string): void { + if (host.startsWith('-')) { + throw new Error(`Refusing to use SSH host that looks like an option: ${host}`); + } +} + /** * Run a command on the SSH host. stdin/stderr stay attached to the terminal * so key passphrase / password prompts and remote progress output work; * stdout is captured for the caller. */ export function runSsh(host: string, remoteCommand: string): RemoteRunResult { + assertSafeSshHost(host); const result = spawnSync('ssh', [host, remoteCommand], { encoding: 'utf8', stdio: ['inherit', 'pipe', 'inherit'], @@ -43,11 +55,18 @@ export function shellQuote(value: string): string { * configured command first and fall back to the default install location. */ export function resolveRemoteSubminerCommand(host: string, preferred: string | null): string { - const candidates = preferred ? [preferred] : ['subminer', '~/.local/bin/subminer']; + // Trusted defaults stay unquoted so the remote shell expands `~`; the + // user-supplied override is shell-quoted to prevent command injection. + const candidates: Array<{ value: string; probe: string }> = preferred + ? [{ value: preferred, probe: shellQuote(preferred) }] + : [ + { value: 'subminer', probe: 'subminer' }, + { value: '~/.local/bin/subminer', probe: '~/.local/bin/subminer' }, + ]; for (const candidate of candidates) { - const probe = runSsh(host, `command -v ${candidate} >/dev/null 2>&1`); + const probe = runSsh(host, `command -v ${candidate.probe} >/dev/null 2>&1`); if (probe.status === 0) { - return candidate; + return candidate.value; } } throw new Error( diff --git a/launcher/sync/sync-db.test.ts b/launcher/sync/sync-db.test.ts index 2923e964..054ad8f3 100644 --- a/launcher/sync/sync-db.test.ts +++ b/launcher/sync/sync-db.test.ts @@ -151,6 +151,65 @@ test('is idempotent: re-merging the same snapshot changes nothing', () => { } }); +test('preserves anilist_id when inserting a new anime from the snapshot', () => { + const { dir, localPath, remotePath } = makeDbPair(); + try { + insertFixtureSession(remotePath, { + uuid: 'remote-1', + videoKey: 'showb-e1', + animeTitleKey: 'showb', + startedAtMs: BASE_MS, + applyLifetime: true, + }); + const remoteDb = new Database(remotePath, { readwrite: true }); + remoteDb.prepare(`UPDATE imm_anime SET anilist_id = 12345 WHERE normalized_title_key = 'showb'`).run(); + remoteDb.close(); + + const summary = mergeSnapshotIntoDb(localPath, remotePath); + assert.equal(summary.animeAdded, 1); + const anime = queryOne<{ anilist_id: number }>( + localPath, + `SELECT anilist_id FROM imm_anime WHERE normalized_title_key = 'showb'`, + ); + assert.equal(anime?.anilist_id, 12345); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + +test('matches an existing local anime by anilist_id even when the title key differs', () => { + const { dir, localPath, remotePath } = makeDbPair(); + try { + insertFixtureSession(localPath, { + uuid: 'local-1', + videoKey: 'showa-e1', + animeTitleKey: 'show-romaji', + startedAtMs: BASE_MS, + applyLifetime: true, + }); + insertFixtureSession(remotePath, { + uuid: 'remote-1', + videoKey: 'showa-e2', + animeTitleKey: 'show-native', + startedAtMs: BASE_MS + DAY_MS, + applyLifetime: true, + }); + const localDb = new Database(localPath, { readwrite: true }); + localDb.prepare(`UPDATE imm_anime SET anilist_id = 999 WHERE normalized_title_key = 'show-romaji'`).run(); + localDb.close(); + const remoteDb = new Database(remotePath, { readwrite: true }); + remoteDb.prepare(`UPDATE imm_anime SET anilist_id = 999 WHERE normalized_title_key = 'show-native'`).run(); + remoteDb.close(); + + const summary = mergeSnapshotIntoDb(localPath, remotePath); + // Same anilist_id → one anime, not two. + assert.equal(summary.animeAdded, 0); + assert.equal(count(localPath, 'SELECT COUNT(*) AS n FROM imm_anime'), 1); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + test('matches shared videos by video_key and merges the watched flag', () => { const { dir, localPath, remotePath } = makeDbPair(); try { diff --git a/launcher/sync/sync-db.ts b/launcher/sync/sync-db.ts index 73cbedb7..ef1fcb3c 100644 --- a/launcher/sync/sync-db.ts +++ b/launcher/sync/sync-db.ts @@ -35,7 +35,13 @@ export function mergeSnapshotIntoDb(localDbPath: string, snapshotPath: string): } const remote = new Database(snapshotPath, { readonly: true }); - const local = new Database(localDbPath, { readwrite: true, create: false }); + let local: Database; + try { + local = new Database(localDbPath, { readwrite: true, create: false }); + } catch (error) { + remote.close(); + throw error; + } try { assertMergeableSchema(remote, 'Snapshot'); assertMergeableSchema(local, 'Local'); diff --git a/launcher/sync/sync-shared.ts b/launcher/sync/sync-shared.ts index 2cbd80b8..09fd6bb5 100644 --- a/launcher/sync/sync-shared.ts +++ b/launcher/sync/sync-shared.ts @@ -121,8 +121,10 @@ function isProcessAlive(pid: number): boolean { try { process.kill(pid, 0); return true; - } catch { - return false; + } catch (error) { + // EPERM means the process exists but we can't signal it → still alive. + // Only ESRCH (no such process) means it's actually gone. + return (error as NodeJS.ErrnoException)?.code === 'EPERM'; } }