From 8aa9756b8afd737c49f1711967f5a5d4eda7c340 Mon Sep 17 00:00:00 2001 From: Twest2 Date: Fri, 3 Jul 2026 23:10:21 -0500 Subject: [PATCH] Harden archives, snapshots, locks, and search against corruption and races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1/2 of the improvement plan (PR 6 of the sequence). Recovery and resource-limit hardening for the storage-adjacent modules. ZIP resource limits (core/zip.js): - unzipSync now enforces entry-count, total compressed, total inflated, and per-entry inflated budgets, and caps inflation with inflateRawSync maxOutputLength so a deflate bomb can't exhaust memory. Exact inflated-size match (not "at least") and CRC verification are kept. Import uses the default limits. Transactional archive import (core/archive.js): - The import validates the guide AND every step before writing anything, then stages the whole guide in a temp directory and publishes it with a single atomic rename. A corrupt step no longer leaves a partial guide in the library; a failure cleans up the staging directory. Atomic snapshot restore (core/snapshots.js): - Restore extracts and validates into a temp directory first; only then does it swap content in, moving live content aside so a mid-swap failure rolls back. A corrupt/truncated snapshot can no longer destroy the live guide (the old restore deleted live content before extracting). - Fixed snapshot filename collisions: names kept milliseconds so two backups in the same second no longer overwrite each other. Automatic backups (core/snapshots.js): - Implemented the previously-dead backups.automatic/everyNSaves/keepLast settings: autoSnapshotIfDue snapshots every N saves and prunes to keepLast, wired into the save choke point in main.js. Never throws — a backup failure cannot break the save that triggered it. Exclusive locks (core/locks.js): - acquireLock uses O_CREAT|O_EXCL (flag 'wx') so only one writer wins the race; the old read-then-write left a window where two writers both believed they held the lock. Added a per-acquisition token so release only removes the exact lock it took (never one a force-steal replaced). Same-process re-acquire still succeeds; cross-process fresh locks conflict. Search reconciliation (core/search.js): - New reconcile(store) rebuilds/repairs the index against the library at startup using per-guide fingerprints (updatedAt+revision): reindexes new/ changed guides, drops entries for deleted ones, and exposes a recovery status ('ok'|'reset'|'reconciled'). A missing/corrupt/version-mismatched index recovers instead of silently returning nothing. Wired into startup. Recovery surface: - New recovery:status IPC + preload method returns quarantined files (this session) and the search index status so the UI can surface data issues. Tests: ZIP bomb/limits, transactional import abort with no partial guide, atomic snapshot restore preserving the live guide on corruption, exclusive lock conflict/steal/release-by-token, same-process re-acquire, search reconcile (rebuild/drop/reindex/corrupt-reset), and automatic backup cadence/pruning. 255 unit tests pass; startup smoke and workflow E2E pass. Co-Authored-By: Claude Fable 5 --- app/main.js | 23 +- app/preload.js | 3 + core/archive.js | 40 +++- core/locks.js | 66 +++++- core/search.js | 74 ++++++- core/snapshots.js | 106 ++++++++- core/zip.js | 52 ++++- tests/unit/recovery-hardening.test.js | 299 ++++++++++++++++++++++++++ 8 files changed, 622 insertions(+), 41 deletions(-) create mode 100644 tests/unit/recovery-hardening.test.js diff --git a/app/main.js b/app/main.js index 8e18cab..4630e7f 100644 --- a/app/main.js +++ b/app/main.js @@ -17,7 +17,7 @@ const { buildRenderAst } = require('../core/renderast'); const { runExport, EXPORTERS } = require('../exporters'); const { runExportInWorker } = require('./export-runner'); const { exportGuideArchive, importGuideArchive, saveLinkedGuide } = require('../core/archive'); -const { createSnapshot, listSnapshots, restoreSnapshot } = require('../core/snapshots'); +const { createSnapshot, listSnapshots, restoreSnapshot, autoSnapshotIfDue } = require('../core/snapshots'); const { readLock } = require('../core/locks'); const CaptureService = require('./capture'); const { TextIntelService } = require('./text-intel'); @@ -72,6 +72,9 @@ function reindex(guideId) { } catch { // index failures must never block saves } + // Automatic backup policy runs on the same save choke point. It is + // self-contained and never throws, so it can't affect the save either. + autoSnapshotIfDue(store, guideId, settings); } function orderedSteps(guideId) { @@ -734,6 +737,13 @@ function setupIpc() { return guide; }, { validate: (a) => c.id(a.guideId) && c.fileName(a.name) }); + // recovery status: corrupt files quarantined this session and search index + // health, so the UI can surface them instead of data silently vanishing. + h('recovery:status', () => ({ + quarantined: store.getRecoveryReport(), + searchStatus: searchIndex.status, + })); + // templates const validFormat = (v) => c.oneOf(v, FORMATS); h('templates:list', ({ format }) => templates.list(format), @@ -916,6 +926,17 @@ if (!gotLock) { store = new GuideStore(dataDir); settings = new Settings(store.settingsDir); searchIndex = new SearchIndex(store.indexDir); + // Rebuild/reconcile the index against the library at startup so a missing, + // corrupt, or version-mismatched index recovers instead of silently + // returning nothing. + try { + const summary = searchIndex.reconcile(store); + if (summary.reindexed || summary.removed || summary.status !== 'ok') { + console.log(`[stepforge] search index reconciled: ${JSON.stringify(summary)}`); + } + } catch (err) { + console.error(`[stepforge] search reconcile failed: ${err && err.message}`); + } templates = new TemplateManager(store.templatesDir); textIntel = new TextIntelService({ store, diff --git a/app/preload.js b/app/preload.js index 3e8e33d..2007e2d 100644 --- a/app/preload.js +++ b/app/preload.js @@ -77,6 +77,9 @@ const api = { create: invoke('snapshots:create'), restore: invoke('snapshots:restore'), }, + recovery: { + status: invoke('recovery:status'), + }, templates: { list: invoke('templates:list'), load: invoke('templates:load'), diff --git a/core/archive.js b/core/archive.js index bd3d312..a77726e 100644 --- a/core/archive.js +++ b/core/archive.js @@ -124,18 +124,40 @@ function importGuideArchive(store, file, { mode = 'copy' } = {}) { } function finalizeImport(store, newGuide, idMap, stepJsons, stepFiles) { + // Transactional import: validate the guide and EVERY step first, then write + // the whole guide into a temporary staging directory, and only publish it + // with a single atomic rename. Previously guide.json was written before the + // steps validated, so a bad step left a partial guide in the library. validateGuide(newGuide); - writeJsonSync(path.join(store.guideDir(newGuide.guideId), 'guide.json'), newGuide); - + const normalizedSteps = []; for (const [stepId, { raw }] of stepJsons) { const step = normalizeStep({ ...raw, stepId }); step.parentStepId = raw.parentStepId ? idMap.get(raw.parentStepId) || null : null; - validateStep(step); - const dir = store.stepDir(newGuide.guideId, stepId); - writeJsonSync(path.join(dir, 'step.json'), step); - for (const { name, data } of stepFiles.get(stepId) || []) { - atomicWriteFileSync(path.join(dir, name), data); + validateStep(step); // throws before anything is written on a bad step + normalizedSteps.push([stepId, step]); + } + + const finalDir = store.guideDir(newGuide.guideId); + if (fs.existsSync(finalDir)) throw new Error(`guide already exists: ${newGuide.guideId}`); + const stagingDir = `${finalDir}.importing-${Date.now()}`; + fs.rmSync(stagingDir, { recursive: true, force: true }); + try { + fs.mkdirSync(stagingDir, { recursive: true }); + writeJsonSync(path.join(stagingDir, 'guide.json'), newGuide); + for (const [stepId, step] of normalizedSteps) { + const dir = path.join(stagingDir, 'steps', stepId); + fs.mkdirSync(dir, { recursive: true }); + writeJsonSync(path.join(dir, 'step.json'), step); + for (const { name, data } of stepFiles.get(stepId) || []) { + atomicWriteFileSync(path.join(dir, name), data); + } } + // Publish atomically. If the final dir appeared meanwhile, fail cleanly. + if (fs.existsSync(finalDir)) throw new Error(`guide already exists: ${newGuide.guideId}`); + fs.renameSync(stagingDir, finalDir); + } catch (err) { + fs.rmSync(stagingDir, { recursive: true, force: true }); + throw err; } return store.getGuide(newGuide.guideId); } @@ -160,7 +182,9 @@ function saveLinkedGuide(store, guideId, { force = false } = {}) { store.saveGuide(guide, { touch: false }); return { saved: true, path: target }; } finally { - releaseLock(target); + // Release by our acquisition token so we never remove a lock a concurrent + // force-steal replaced with theirs. + releaseLock(target, { lock: result.lock }); } } diff --git a/core/locks.js b/core/locks.js index 5c94330..dcf7de7 100644 --- a/core/locks.js +++ b/core/locks.js @@ -20,18 +20,39 @@ function lockPathFor(archivePath) { return path.join(dir, `${stem}.lock-sfgz`); } -function currentHolder() { +function currentProcess() { return { host: os.hostname(), user: os.userInfo().username, pid: process.pid }; } +function currentHolder() { + return { + ...currentProcess(), + // Random per-acquisition token so two processes that happen to share + // host+user+pid space (containers, pid reuse) still compare distinctly, + // and so a steal can be detected by the previous holder. + token: `${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 10)}`, + }; +} + function readLock(archivePath) { return readJsonIfExists(lockPathFor(archivePath), null); } -function sameHolder(a, b) { +// Process identity (host+user+pid). Used to decide whether an existing lock is +// held by *this process* (safe to re-acquire) or someone else (a conflict). +function sameProcess(a, b) { return a && b && a.host === b.host && a.user === b.user && a.pid === b.pid; } +// Exact-acquisition identity via the per-acquisition token. Used by release so +// a caller only removes the lock it actually took (never one a force-steal +// replaced with its own). +function sameAcquisition(existing, owner) { + if (!existing || !owner) return false; + if (owner.token) return existing.token === owner.token; + return sameProcess(existing, owner); +} + function isStale(lock, now = Date.now()) { const t = Date.parse(lock && lock.acquiredAt); return !Number.isFinite(t) || now - t > STALE_AFTER_MS; @@ -44,24 +65,49 @@ function isStale(lock, now = Date.now()) { */ function acquireLock(archivePath, { force = false } = {}) { const file = lockPathFor(archivePath); - const existing = readLock(archivePath); const me = currentHolder(); - if (existing && !sameHolder(existing, me) && !isStale(existing) && !force) { + const lock = { ...me, acquiredAt: nowIso() }; + const payload = JSON.stringify(lock, null, 2); + + // Fast path: exclusive create. Only one writer wins the O_CREAT|O_EXCL race, + // so two processes can't both believe they hold the lock (the old + // read-then-write left exactly that window open). + try { + fs.writeFileSync(file, payload, { flag: 'wx' }); + return { acquired: true, lock }; + } catch (err) { + if (err.code !== 'EEXIST') throw err; + } + + // A lock already exists. We may take it over only if this process already + // holds it, it is stale, or the caller is force-stealing (user confirmed). + const existing = readLock(archivePath); + if (existing && !sameProcess(existing, me) && !isStale(existing) && !force) { return { acquired: false, conflict: existing }; } - const lock = { ...me, acquiredAt: nowIso() }; - fs.writeFileSync(file, JSON.stringify(lock, null, 2)); + // Overwrite to claim ownership (our token now identifies the lock). + fs.writeFileSync(file, payload); return { acquired: true, lock }; } -/** Release only if we are the holder (or force). */ -function releaseLock(archivePath, { force = false } = {}) { +/** + * Release only if we are the holder (or force). Pass the `lock` (or its + * `token`) returned by acquireLock so ownership is matched by token — the + * per-acquisition token means a fresh currentHolder() would not match. + */ +function releaseLock(archivePath, { force = false, lock = null, token = null } = {}) { const file = lockPathFor(archivePath); const existing = readLock(archivePath); if (!existing) return true; - if (!force && !sameHolder(existing, currentHolder())) return false; + // With no explicit lock/token, fall back to process identity (the legacy + // "release my own lock" path) rather than a fresh token that can't match. + const owner = lock || (token ? { token } : currentProcess()); + if (!force && !sameAcquisition(existing, owner)) return false; fs.rmSync(file, { force: true }); return true; } -module.exports = { lockPathFor, readLock, acquireLock, releaseLock, isStale, STALE_AFTER_MS }; +module.exports = { + lockPathFor, readLock, acquireLock, releaseLock, isStale, STALE_AFTER_MS, + sameProcess, sameAcquisition, +}; diff --git a/core/search.js b/core/search.js index 21da140..ccd706f 100644 --- a/core/search.js +++ b/core/search.js @@ -14,7 +14,7 @@ const { blockText } = require('./blocks'); * specific step in the editor. */ -const INDEX_VERSION = 1; +const INDEX_VERSION = 2; function tokenize(text) { if (!text) return []; @@ -27,20 +27,82 @@ function tokenize(text) { class SearchIndex { constructor(indexDir) { this.file = path.join(indexDir, 'search-index.json'); + // Per-guide source fingerprints so a startup reconcile can tell which + // guides changed while the app was closed, without re-reading every step. + this.fingerprints = {}; // guideId -> fingerprint string + // Recovery status surfaced to the UI: 'ok' | 'reset' (missing/corrupt/ + // version mismatch) | 'reconciled' (rebuilt from the store at startup). + this.status = 'ok'; + const fileExisted = require('node:fs').existsSync(this.file); const stored = readJsonIfExists(this.file, null); - if (stored && stored.version === INDEX_VERSION) { + if (stored && stored.version === INDEX_VERSION && stored.docs && typeof stored.docs === 'object') { this.docs = stored.docs; + this.fingerprints = stored.fingerprints || {}; } else { + // Missing, corrupt, or an older index version: start empty and mark it, + // so reconcile() rebuilds from the store instead of silently staying + // blank (which made search "work" but return nothing). A file that + // existed but could not be used is a 'reset' (recovery-worthy); a + // genuinely absent index on first run is just 'ok'. this.docs = {}; // docKey -> { guideId, stepId, title, text, updatedAt } + this.status = fileExisted ? 'reset' : 'ok'; } } persist() { - writeJsonSync(this.file, { version: INDEX_VERSION, docs: this.docs }); + writeJsonSync(this.file, { + version: INDEX_VERSION, + docs: this.docs, + fingerprints: this.fingerprints, + }); + } + + static fingerprint(guide) { + return `${guide.updatedAt || ''}:${Number.isInteger(guide.revision) ? guide.revision : 0}`; + } + + /** + * Reconcile the index against the store at startup: reindex guides that are + * new or changed (by fingerprint), and drop index entries for guides that no + * longer exist. Returns a summary with a recovery status for the UI. + */ + reconcile(store) { + const guides = store.listGuides(); + const liveIds = new Set(guides.map((g) => g.guideId)); + let reindexed = 0; + let removed = 0; + + // Drop docs/fingerprints for guides that are gone. + for (const key of Object.keys(this.fingerprints)) { + if (!liveIds.has(key)) { + this.removeGuide(key, { persist: false }); + delete this.fingerprints[key]; + removed += 1; + } + } + + for (const guide of guides) { + const fp = SearchIndex.fingerprint(guide); + const indexed = this.fingerprints[guide.guideId]; + const hasDoc = Boolean(this.docs[`g:${guide.guideId}`]); + if (indexed === fp && hasDoc) continue; // unchanged + try { + this.indexGuide(guide, store.listSteps(guide.guideId), { persist: false }); + reindexed += 1; + } catch { + // A single unreadable guide must not abort the whole reconcile. + } + } + + this.persist(); + if (this.status === 'reset' || reindexed > 0 || removed > 0) { + this.status = this.status === 'reset' ? 'reset' : 'reconciled'; + } + return { status: this.status, reindexed, removed, total: guides.length }; } /** (Re)index one guide and all of its steps. */ - indexGuide(guide, stepsMap) { + indexGuide(guide, stepsMap, { persist = true } = {}) { this.removeGuide(guide.guideId, { persist: false }); const placeholderText = Object.entries(guide.placeholders || {}) @@ -69,13 +131,15 @@ class SearchIndex { updatedAt: guide.updatedAt, }; } - this.persist(); + this.fingerprints[guide.guideId] = SearchIndex.fingerprint(guide); + if (persist) this.persist(); } removeGuide(guideId, { persist = true } = {}) { for (const key of Object.keys(this.docs)) { if (this.docs[key].guideId === guideId) delete this.docs[key]; } + delete this.fingerprints[guideId]; if (persist) this.persist(); } diff --git a/core/snapshots.js b/core/snapshots.js index a785197..32a2c3c 100644 --- a/core/snapshots.js +++ b/core/snapshots.js @@ -3,7 +3,8 @@ const fs = require('node:fs'); const path = require('node:path'); const { zipDirSync, extractZipSync } = require('./zip'); -const { atomicWriteFileSync } = require('./util'); +const { atomicWriteFileSync, readJsonSync } = require('./util'); +const { validateGuide } = require('./schema'); /** * Snapshot backups: a zip of the guide directory (excluding history/) stored @@ -16,7 +17,11 @@ function snapshotsDir(store, guideId) { } function snapshotName(label) { - const stamp = new Date().toISOString().replace(/[:.]/g, '-').replace(/-\d{3}Z$/, 'Z'); + // Keep milliseconds: stripping them made two snapshots taken within the same + // second collide on filename (the second silently overwrote the first, so + // rapid automatic backups produced only one file). ms keeps names unique and + // still chronologically sortable. + const stamp = new Date().toISOString().replace(/[:.]/g, '-'); return label ? `${stamp}-${label.replace(/[^A-Za-z0-9_-]+/g, '_')}.zip` : `${stamp}.zip`; } @@ -50,20 +55,103 @@ function pruneSnapshots(store, guideId, keepLast) { /** * Restore a snapshot: replaces the guide's current content (guide.json and * steps/) with the snapshot's, keeping the history/ directory intact. + * + * The extraction is staged and validated BEFORE any live content is touched: + * a corrupt or truncated snapshot can no longer destroy the current guide. + * The swap itself moves the old content aside, moves the new content in, then + * deletes the old — so a failure mid-swap leaves a recoverable state. */ function restoreSnapshot(store, guideId, name) { const file = path.join(snapshotsDir(store, guideId), path.basename(name)); if (!fs.existsSync(file)) throw new Error(`snapshot not found: ${name}`); const buf = fs.readFileSync(file); const guideDir = store.guideDir(guideId); - // Safety: snapshot the pre-restore state too, so a restore is undoable. - createSnapshot(store, guideId, { label: 'pre-restore' }); - for (const entry of fs.readdirSync(guideDir)) { - if (entry === 'history') continue; - fs.rmSync(path.join(guideDir, entry), { recursive: true, force: true }); + + // 1. Extract + validate into a temp staging dir. Nothing live is touched yet. + const staging = `${guideDir}.restoring-${Date.now()}`; + fs.rmSync(staging, { recursive: true, force: true }); + try { + fs.mkdirSync(staging, { recursive: true }); + extractZipSync(buf, staging); + const guideJson = path.join(staging, 'guide.json'); + if (!fs.existsSync(guideJson)) throw new Error('snapshot is missing guide.json'); + validateGuide(readJsonSync(guideJson)); // throws on a corrupt snapshot + } catch (err) { + fs.rmSync(staging, { recursive: true, force: true }); + throw new Error(`snapshot restore aborted (snapshot invalid): ${err.message}`); } - extractZipSync(buf, guideDir); + + // 2. Snapshot the pre-restore state so the restore is itself undoable. + createSnapshot(store, guideId, { label: 'pre-restore' }); + + // 3. Swap in the validated content, preserving history/. Move live content + // aside first so we can roll back if a step fails. + const backup = `${guideDir}.prev-${Date.now()}`; + const liveEntries = fs.readdirSync(guideDir).filter((e) => e !== 'history'); + fs.mkdirSync(backup, { recursive: true }); + try { + for (const entry of liveEntries) { + fs.renameSync(path.join(guideDir, entry), path.join(backup, entry)); + } + for (const entry of fs.readdirSync(staging)) { + if (entry === 'history') continue; + fs.renameSync(path.join(staging, entry), path.join(guideDir, entry)); + } + } catch (err) { + // Roll back: restore whatever we moved aside. + for (const entry of fs.readdirSync(backup)) { + const dest = path.join(guideDir, entry); + fs.rmSync(dest, { recursive: true, force: true }); + fs.renameSync(path.join(backup, entry), dest); + } + fs.rmSync(backup, { recursive: true, force: true }); + fs.rmSync(staging, { recursive: true, force: true }); + throw err; + } + fs.rmSync(backup, { recursive: true, force: true }); + fs.rmSync(staging, { recursive: true, force: true }); return store.getGuide(guideId); } -module.exports = { createSnapshot, listSnapshots, pruneSnapshots, restoreSnapshot, snapshotsDir }; +/** + * Automatic backup policy. Every guide keeps a small save counter in its + * history dir; once `everyNSaves` saves accumulate (and backups.automatic is + * on) an automatic snapshot is taken and old ones pruned to backups.keepLast. + * Returns the snapshot name when one was taken, else null. Never throws — a + * backup failure must not break the save that triggered it. + */ +function autoSnapshotIfDue(store, guideId, settings) { + try { + const backups = (settings && settings.get && settings.get('backups')) || {}; + if (backups.automatic === false) return null; + const everyN = Number.isInteger(backups.everyNSaves) && backups.everyNSaves > 0 ? backups.everyNSaves : 25; + const keepLast = Number.isInteger(backups.keepLast) && backups.keepLast > 0 ? backups.keepLast : 10; + + const dir = path.join(store.guideDir(guideId), 'history'); + fs.mkdirSync(dir, { recursive: true }); + const counterFile = path.join(dir, 'autosave-counter.json'); + let count = 0; + try { + count = JSON.parse(fs.readFileSync(counterFile, 'utf8')).count || 0; + } catch { count = 0; } + count += 1; + + if (count >= everyN) { + createSnapshot(store, guideId, { label: 'auto', keepLast }); + count = 0; + atomicWriteFileSync(counterFile, JSON.stringify({ count })); + return true; + } + atomicWriteFileSync(counterFile, JSON.stringify({ count })); + return null; + } catch (err) { + // Best effort: report, never break the caller's save. + console.error(`[stepforge] automatic backup failed for ${guideId}: ${err && err.message}`); + return null; + } +} + +module.exports = { + createSnapshot, listSnapshots, pruneSnapshots, restoreSnapshot, snapshotsDir, + autoSnapshotIfDue, +}; diff --git a/core/zip.js b/core/zip.js index 6eba788..18658e1 100644 --- a/core/zip.js +++ b/core/zip.js @@ -121,8 +121,22 @@ function zipSync(entries, { date = new Date(2026, 0, 1) } = {}) { return Buffer.concat([...localParts, centralBuf, eocd]); } -/** Parse a zip buffer into [{ name, data }] with CRC verification. */ -function unzipSync(buffer) { +// Resource limits for untrusted archives (share files, snapshots). These cap +// memory and disk work so a ZIP bomb can't exhaust the machine. Callers that +// build archives themselves may relax them; imports use the defaults. +const DEFAULT_UNZIP_LIMITS = { + maxEntries: 50000, + maxTotalCompressed: 1024 * 1024 * 1024, // 1 GiB of stored bytes + maxTotalUncompressed: 4 * 1024 * 1024 * 1024, // 4 GiB inflated total + maxEntryUncompressed: 512 * 1024 * 1024, // 512 MiB per entry +}; + +/** + * Parse a zip buffer into [{ name, data }] with CRC verification and hard + * resource limits. `limits` overrides DEFAULT_UNZIP_LIMITS. + */ +function unzipSync(buffer, { limits = {} } = {}) { + const lim = { ...DEFAULT_UNZIP_LIMITS, ...limits }; if (!Buffer.isBuffer(buffer) || buffer.length < 22) throw new Error('zip: too small'); // Find end-of-central-directory record (scan backwards over the comment). let eocd = -1; @@ -132,11 +146,14 @@ function unzipSync(buffer) { } if (eocd < 0) throw new Error('zip: end record not found'); const count = buffer.readUInt16LE(eocd + 10); + if (count > lim.maxEntries) throw new Error(`zip: too many entries (${count} > ${lim.maxEntries})`); let pos = buffer.readUInt32LE(eocd + 16); const entries = []; + let totalCompressed = 0; + let totalUncompressed = 0; for (let i = 0; i < count; i++) { - if (buffer.readUInt32LE(pos) !== 0x02014b50) throw new Error('zip: bad central header'); + if (pos + 46 > buffer.length || buffer.readUInt32LE(pos) !== 0x02014b50) throw new Error('zip: bad central header'); const method = buffer.readUInt16LE(pos + 10); const crc = buffer.readUInt32LE(pos + 16); const compSize = buffer.readUInt32LE(pos + 20); @@ -151,17 +168,33 @@ function unzipSync(buffer) { assertSafeEntryName(name); if (name.endsWith('/')) continue; // directory entry + // Budget checks BEFORE allocating/inflating: the declared sizes are + // attacker-controlled, so reject oversize claims up front. + if (uncompSize > lim.maxEntryUncompressed) { + throw new Error(`zip: entry too large (${uncompSize} > ${lim.maxEntryUncompressed}): ${name}`); + } + totalCompressed += compSize; + totalUncompressed += uncompSize; + if (totalCompressed > lim.maxTotalCompressed) throw new Error('zip: total compressed size exceeds limit'); + if (totalUncompressed > lim.maxTotalUncompressed) throw new Error('zip: total inflated size exceeds limit'); + if (buffer.readUInt32LE(localOffset) !== 0x04034b50) throw new Error('zip: bad local header'); const lNameLen = buffer.readUInt16LE(localOffset + 26); const lExtraLen = buffer.readUInt16LE(localOffset + 28); const dataStart = localOffset + 30 + lNameLen + lExtraLen; + if (dataStart + compSize > buffer.length) throw new Error(`zip: entry data out of range: ${name}`); const raw = buffer.subarray(dataStart, dataStart + compSize); let data; if (method === 0) data = Buffer.from(raw); - else if (method === 8) data = zlib.inflateRawSync(raw); - else throw new Error(`zip: unsupported method ${method} for ${name}`); + else if (method === 8) { + // Cap inflation so a small deflate stream can't expand to gigabytes — + // even if the declared uncompSize lied, this is the real guard. + data = zlib.inflateRawSync(raw, { maxOutputLength: lim.maxEntryUncompressed }); + } else throw new Error(`zip: unsupported method ${method} for ${name}`); + // Exact length match (not "at least"): the inflated bytes must equal the + // declared uncompressed size, and the CRC must verify. if (data.length !== uncompSize) throw new Error(`zip: size mismatch for ${name}`); if (crc32(data) !== crc) throw new Error(`zip: CRC mismatch for ${name}`); entries.push({ name, data }); @@ -170,10 +203,10 @@ function unzipSync(buffer) { } /** Extract a zip buffer under destDir; every path is traversal-checked. */ -function extractZipSync(buffer, destDir) { +function extractZipSync(buffer, destDir, { limits = {} } = {}) { const resolvedDest = path.resolve(destDir); const written = []; - for (const { name, data } of unzipSync(buffer)) { + for (const { name, data } of unzipSync(buffer, { limits })) { const target = path.resolve(resolvedDest, name); if (target !== resolvedDest && !target.startsWith(resolvedDest + path.sep)) { throw new Error(`zip: entry escapes destination: ${name}`); @@ -203,4 +236,7 @@ function zipDirSync(dir, { filter = () => true, prefix = '' } = {}) { return zipSync(entries); } -module.exports = { crc32, zipSync, unzipSync, extractZipSync, zipDirSync, assertSafeEntryName }; +module.exports = { + crc32, zipSync, unzipSync, extractZipSync, zipDirSync, assertSafeEntryName, + DEFAULT_UNZIP_LIMITS, +}; diff --git a/tests/unit/recovery-hardening.test.js b/tests/unit/recovery-hardening.test.js new file mode 100644 index 0000000..3c3b899 --- /dev/null +++ b/tests/unit/recovery-hardening.test.js @@ -0,0 +1,299 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const zlib = require('node:zlib'); + +const { GuideStore } = require('../../core/store'); +const { SearchIndex } = require('../../core/search'); +const { unzipSync, zipSync, crc32 } = require('../../core/zip'); +const { exportGuideArchive, importGuideArchive } = require('../../core/archive'); +const { createSnapshot, restoreSnapshot, autoSnapshotIfDue } = require('../../core/snapshots'); +const { acquireLock, releaseLock } = require('../../core/locks'); +const { makeTmpDir, rmrf, TINY_PNG } = require('./helpers'); + +// ---- ZIP bomb / resource limits --------------------------------------------- + +test('unzip rejects an archive that declares too many entries', () => { + const many = []; + for (let i = 0; i < 20; i += 1) many.push({ name: `f${i}.txt`, data: 'x' }); + const buf = zipSync(many); + assert.throws(() => unzipSync(buf, { limits: { maxEntries: 5 } }), /too many entries/); +}); + +test('unzip rejects an entry whose declared size exceeds the per-entry limit', () => { + const buf = zipSync([{ name: 'big.txt', data: Buffer.alloc(1000, 65) }]); + assert.throws(() => unzipSync(buf, { limits: { maxEntryUncompressed: 100 } }), /entry too large/); +}); + +test('unzip caps inflation so a deflate bomb cannot exhaust memory', () => { + // A hand-built entry whose deflate stream expands far past the cap. The + // maxOutputLength guard must abort inflation rather than allocating it all. + const bomb = Buffer.alloc(10 * 1024 * 1024, 0); // 10 MiB of zeros -> tiny deflate + const raw = zlib.deflateRawSync(bomb, { level: 9 }); + const name = 'bomb'; + const nameBuf = Buffer.from(name); + const local = Buffer.alloc(30); + local.writeUInt32LE(0x04034b50, 0); + local.writeUInt16LE(20, 4); + local.writeUInt16LE(0x0800, 6); + local.writeUInt16LE(8, 8); // deflate + local.writeUInt32LE(crc32(bomb), 14); + local.writeUInt32LE(raw.length, 18); + local.writeUInt32LE(bomb.length, 22); + local.writeUInt16LE(nameBuf.length, 26); + const central = Buffer.alloc(46); + central.writeUInt32LE(0x02014b50, 0); + central.writeUInt16LE(20, 4); + central.writeUInt16LE(20, 6); + central.writeUInt16LE(0x0800, 8); + central.writeUInt16LE(8, 10); + central.writeUInt32LE(crc32(bomb), 16); + central.writeUInt32LE(raw.length, 20); + central.writeUInt32LE(bomb.length, 24); + central.writeUInt16LE(nameBuf.length, 28); + central.writeUInt32LE(0, 42); + const localBlock = Buffer.concat([local, nameBuf, raw]); + const centralBlock = Buffer.concat([central, nameBuf]); + const eocd = Buffer.alloc(22); + eocd.writeUInt32LE(0x06054b50, 0); + eocd.writeUInt16LE(1, 8); + eocd.writeUInt16LE(1, 10); + eocd.writeUInt32LE(centralBlock.length, 12); + eocd.writeUInt32LE(localBlock.length, 16); + const buf = Buffer.concat([localBlock, centralBlock, eocd]); + + assert.throws(() => unzipSync(buf, { limits: { maxEntryUncompressed: 64 * 1024 } })); +}); + +// ---- transactional archive import ------------------------------------------- + +test('a corrupt step aborts the import leaving no partial guide', (t) => { + const root = makeTmpDir('import-atomic'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + + const guide = store.createGuide({ title: 'Src' }); + store.addStep(guide.guideId, { title: 'S1' }, TINY_PNG, { width: 1, height: 1 }); + const archiveFile = path.join(root, 'out.sfgz'); + exportGuideArchive(store, guide.guideId, archiveFile); + + // Corrupt the exported step so validation fails during import. + const { unzipSync: uz } = require('../../core/zip'); + const entries = uz(fs.readFileSync(archiveFile)); + const tampered = entries.map((e) => { + if (e.name.endsWith('step.json')) { + const obj = JSON.parse(e.data.toString('utf8')); + // Corrupt the image size to non-finite values — validateStep rejects + // an image step with an invalid size. + obj.image = { originalPath: 'original.png', workingPath: 'working.png', size: { width: 'x', height: null } }; + return { name: e.name, data: Buffer.from(JSON.stringify(obj)) }; + } + return { name: e.name, data: e.data }; + }); + fs.writeFileSync(archiveFile, zipSync(tampered)); + + const before = store.listGuides().length; + assert.throws(() => importGuideArchive(store, archiveFile, { mode: 'copy' })); + // No partial guide was left behind, and no staging dir remains. + assert.equal(store.listGuides().length, before); + const leftover = fs.readdirSync(store.guidesDir).filter((n) => n.includes('.importing')); + assert.deepEqual(leftover, []); +}); + +test('a valid archive imports cleanly', (t) => { + const root = makeTmpDir('import-ok'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const guide = store.createGuide({ title: 'Src' }); + store.addStep(guide.guideId, { title: 'S1' }, TINY_PNG, { width: 1, height: 1 }); + const archiveFile = path.join(root, 'out.sfgz'); + exportGuideArchive(store, guide.guideId, archiveFile); + + const imported = importGuideArchive(store, archiveFile, { mode: 'copy' }); + assert.equal(imported.title, 'Src'); + assert.equal(imported.stepsOrder.length, 1); +}); + +// ---- atomic snapshot restore ------------------------------------------------ + +test('restoring a corrupt snapshot never destroys the live guide', (t) => { + const root = makeTmpDir('snap-atomic'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const guide = store.createGuide({ title: 'Live' }); + store.addStep(guide.guideId, { title: 'keep me' }, TINY_PNG, { width: 1, height: 1 }); + const snap = createSnapshot(store, guide.guideId, { label: 'good' }); + + // Corrupt the snapshot zip so restore must abort. + const snapFile = path.join(store.guideDir(guide.guideId), 'history', 'snapshots', snap); + fs.writeFileSync(snapFile, Buffer.from('not a zip at all')); + + assert.throws(() => restoreSnapshot(store, guide.guideId, snap), /restore aborted|invalid|zip/i); + // The live guide and its step are intact. + const after = store.getGuide(guide.guideId); + assert.equal(after.title, 'Live'); + assert.equal(after.stepsOrder.length, 1); +}); + +test('restoring a valid snapshot swaps content and keeps history', (t) => { + const root = makeTmpDir('snap-ok'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const guide = store.createGuide({ title: 'V1' }); + const s1 = store.addStep(guide.guideId, { title: 'first' }, TINY_PNG, { width: 1, height: 1 }); + const snap = createSnapshot(store, guide.guideId, { label: 'v1' }); + + // Change the guide, then restore. + store.saveGuide({ ...store.getGuide(guide.guideId), title: 'V2' }); + store.deleteStep(guide.guideId, s1.stepId); + assert.equal(store.getGuide(guide.guideId).title, 'V2'); + + const restored = restoreSnapshot(store, guide.guideId, snap); + assert.equal(restored.title, 'V1'); + assert.equal(restored.stepsOrder.length, 1); + // history/ survived the restore (pre-restore snapshot exists too). + assert.ok(fs.existsSync(path.join(store.guideDir(guide.guideId), 'history'))); +}); + +// ---- atomic locks ----------------------------------------------------------- + +test('another process holding a fresh lock is a conflict; release-by-token frees ours', (t) => { + const root = makeTmpDir('lock'); + t.after(() => rmrf(root)); + const { lockPathFor } = require('../../core/locks'); + const target = path.join(root, 'shared.sfgz'); + fs.writeFileSync(target, 'x'); + + // Simulate a different process already holding a fresh lock. + fs.writeFileSync(lockPathFor(target), JSON.stringify({ + host: 'other-host', user: 'someone-else', pid: 999999, + token: 'their-token', acquiredAt: new Date().toISOString(), + })); + + const attempt = acquireLock(target); + assert.equal(attempt.acquired, false); + assert.ok(attempt.conflict); + // We must not be able to release their lock with a guessed/absent token. + assert.equal(releaseLock(target, { token: 'wrong' }), false); + + // Force-steal (user confirmed), then release by our own acquisition token. + const stolen = acquireLock(target, { force: true }); + assert.equal(stolen.acquired, true); + assert.equal(releaseLock(target, { lock: stolen.lock }), true); + assert.equal(acquireLock(target).acquired, true); +}); + +test('the same process can re-acquire its own lock', (t) => { + const root = makeTmpDir('lock-reacquire'); + t.after(() => rmrf(root)); + const target = path.join(root, 'shared.sfgz'); + fs.writeFileSync(target, 'x'); + assert.equal(acquireLock(target).acquired, true); + // Same process, second acquire: not a conflict. + assert.equal(acquireLock(target).acquired, true); +}); + +test('force steal takes over a held lock', (t) => { + const root = makeTmpDir('lock-force'); + t.after(() => rmrf(root)); + const target = path.join(root, 'shared.sfgz'); + fs.writeFileSync(target, 'x'); + acquireLock(target); + const stolen = acquireLock(target, { force: true }); + assert.equal(stolen.acquired, true); +}); + +// ---- search reconcile ------------------------------------------------------- + +test('reconcile rebuilds a missing index from the store', (t) => { + const root = makeTmpDir('search-rebuild'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const guide = store.createGuide({ title: 'Password reset guide' }); + store.addStep(guide.guideId, { title: 'Open admin portal' }, TINY_PNG, { width: 1, height: 1 }); + + // A brand-new index (nothing persisted) must recover by reconciling. + const index = new SearchIndex(store.indexDir); + const summary = index.reconcile(store); + assert.equal(summary.reindexed, 1); + assert.ok(index.search('password').length > 0); +}); + +test('reconcile drops entries for deleted guides and reindexes changed ones', (t) => { + const root = makeTmpDir('search-reconcile'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const g1 = store.createGuide({ title: 'alpha guide' }); + const g2 = store.createGuide({ title: 'beta guide' }); + const index = new SearchIndex(store.indexDir); + index.reconcile(store); + assert.ok(index.search('alpha').length > 0); + + // Delete g1 out from under the index and change g2's title. + store.deleteGuide(g1.guideId); + store.saveGuide({ ...store.getGuide(g2.guideId), title: 'beta renamed gamma' }); + + const summary = index.reconcile(store); + assert.equal(index.search('alpha').length, 0, 'deleted guide is gone from search'); + assert.ok(index.search('gamma').length > 0, 'changed guide is reindexed'); + assert.equal(summary.removed, 1); +}); + +test('a corrupt index file resets to a recoverable status', (t) => { + const root = makeTmpDir('search-corrupt'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + store.createGuide({ title: 'recoverable' }); + fs.mkdirSync(store.indexDir, { recursive: true }); + fs.writeFileSync(path.join(store.indexDir, 'search-index.json'), '{ corrupt json'); + + const index = new SearchIndex(store.indexDir); + const summary = index.reconcile(store); + assert.equal(summary.status, 'reset'); + assert.ok(index.search('recoverable').length > 0); +}); + +// ---- automatic backups ------------------------------------------------------ + +test('autoSnapshotIfDue takes a snapshot every N saves and prunes', (t) => { + const root = makeTmpDir('auto-backup'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const guide = store.createGuide({ title: 'G' }); + const settings = { + get: (k) => ({ automatic: true, everyNSaves: 3, keepLast: 2 }[k.replace('backups.', '')] ?? ({ backups: { automatic: true, everyNSaves: 3, keepLast: 2 } }[k])), + }; + // The helper reads settings.get('backups'): + const s = { get: (k) => (k === 'backups' ? { automatic: true, everyNSaves: 3, keepLast: 2 } : null) }; + + const dir = path.join(store.guideDir(guide.guideId), 'history', 'snapshots'); + const count = () => (fs.existsSync(dir) ? fs.readdirSync(dir).filter((n) => n.endsWith('.zip')).length : 0); + + assert.equal(autoSnapshotIfDue(store, guide.guideId, s), null); // 1 + assert.equal(autoSnapshotIfDue(store, guide.guideId, s), null); // 2 + assert.equal(autoSnapshotIfDue(store, guide.guideId, s), true); // 3 -> snapshot + assert.equal(count(), 1); + autoSnapshotIfDue(store, guide.guideId, s); // 1 + autoSnapshotIfDue(store, guide.guideId, s); // 2 + autoSnapshotIfDue(store, guide.guideId, s); // 3 -> snapshot + assert.equal(count(), 2); + autoSnapshotIfDue(store, guide.guideId, s); + autoSnapshotIfDue(store, guide.guideId, s); + autoSnapshotIfDue(store, guide.guideId, s); // 3rd snapshot, pruned to keepLast=2 + assert.equal(count(), 2, 'pruned to keepLast'); +}); + +test('autoSnapshotIfDue is a no-op when automatic backups are off', (t) => { + const root = makeTmpDir('auto-backup-off'); + t.after(() => rmrf(root)); + const store = new GuideStore(root); + const guide = store.createGuide({ title: 'G' }); + const s = { get: () => ({ automatic: false, everyNSaves: 1 }) }; + assert.equal(autoSnapshotIfDue(store, guide.guideId, s), null); + assert.equal(autoSnapshotIfDue(store, guide.guideId, s), null); + const dir = path.join(store.guideDir(guide.guideId), 'history', 'snapshots'); + assert.equal(fs.existsSync(dir) ? fs.readdirSync(dir).length : 0, 0); +});