fix(persist): stop swallowing save errors + decouple .bak refresh from save

Two related fixes from the deep-audit pass:

HIGH-2: save-global-settings-sync used `try {} catch {}` and always
returned `event.returnValue = true`, so a disk-full / AV-lock /
permissions failure looked like success to the renderer's beforeunload
chain. The user closes the app, comes back, settings are gone —
without any indication. Now the catch sets returnValue=false and
debugLogs the error message, and the bak refresh is in its own
nested try so a transient lock there doesn't fail the whole save.

MED-4: lib/config-store.js _atomicWrite had the same TOCTOU on the
.bak refresh — fs.existsSync(...) then fs.readFileSync(...) without
guarding the read. Wrapped the read+write of the backup in its own
try/catch: a stale .bak is preferable to dropping the new write
entirely just because Windows Defender briefly locked the file mid-
operation. The rename of tmp → live still throws on real failure,
which is what the outer reject is for.

119/119 tests still green; both fixes are defensive guards on
already-tested write paths.
This commit is contained in:
Administrator 2026-04-28 09:40:08 +02:00
parent 3626978250
commit a6958f1418
3 changed files with 31 additions and 13 deletions

View File

@ -250,6 +250,11 @@ class ConfigStore {
const backupPath = this.filePath + '.bak'; const backupPath = this.filePath + '.bak';
fs.writeFile(tmpPath, data, 'utf-8', (err) => { fs.writeFile(tmpPath, data, 'utf-8', (err) => {
if (err) return reject(err); if (err) return reject(err);
try {
// Refresh .bak from the previous live file. Wrapped in try/catch
// so an AV/indexer briefly locking the file doesn't fail the whole
// save — the rename to the live path is the part that matters,
// a stale .bak is preferable to losing the new write entirely.
try { try {
if (fs.existsSync(this.filePath)) { if (fs.existsSync(this.filePath)) {
const existing = fs.readFileSync(this.filePath, 'utf-8'); const existing = fs.readFileSync(this.filePath, 'utf-8');
@ -257,6 +262,7 @@ class ConfigStore {
fs.writeFileSync(backupPath, existing, 'utf-8'); fs.writeFileSync(backupPath, existing, 'utf-8');
} }
} }
} catch {}
fs.renameSync(tmpPath, this.filePath); fs.renameSync(tmpPath, this.filePath);
} catch (e) { return reject(e); } } catch (e) { return reject(e); }
resolve(); resolve();

17
main.js
View File

@ -1616,7 +1616,10 @@ ipcMain.handle('save-global-settings', async (_event, globalSettings) => {
}); });
// Synchronous save for beforeunload — blocks renderer until write completes // Synchronous save for beforeunload — blocks renderer until write completes
// Uses atomic write pattern (tmp + backup + rename) to prevent corruption // Uses atomic write pattern (tmp + backup + rename) to prevent corruption.
// Returns false on any failure so the renderer (which surfaces this via the
// beforeunload chain) doesn't quietly think queue + settings persisted when
// they didn't. Errors are logged for diagnostics regardless.
ipcMain.on('save-global-settings-sync', (event, globalSettings) => { ipcMain.on('save-global-settings-sync', (event, globalSettings) => {
try { try {
const current = configStore.load(); const current = configStore.load();
@ -1626,14 +1629,24 @@ ipcMain.on('save-global-settings-sync', (event, globalSettings) => {
const backupPath = configStore.filePath + '.bak'; const backupPath = configStore.filePath + '.bak';
fs.writeFileSync(tmpPath, data, 'utf-8'); fs.writeFileSync(tmpPath, data, 'utf-8');
if (fs.existsSync(configStore.filePath)) { if (fs.existsSync(configStore.filePath)) {
// Use try/catch around the read so an AV/lock race doesn't fail the
// whole save just because we couldn't refresh the .bak — the write to
// the live file via rename is what matters.
try {
const existing = fs.readFileSync(configStore.filePath, 'utf-8'); const existing = fs.readFileSync(configStore.filePath, 'utf-8');
if (existing && existing.trim().length > 2) { if (existing && existing.trim().length > 2) {
fs.writeFileSync(backupPath, existing, 'utf-8'); fs.writeFileSync(backupPath, existing, 'utf-8');
} }
} catch (bakErr) {
debugLog(`save-global-settings-sync: backup read/write skipped: ${bakErr.message}`);
}
} }
fs.renameSync(tmpPath, configStore.filePath); fs.renameSync(tmpPath, configStore.filePath);
} catch {}
event.returnValue = true; event.returnValue = true;
} catch (err) {
debugLog(`save-global-settings-sync FAILED: ${err && err.message ? err.message : err}`);
event.returnValue = false;
}
}); });
// --- Folder Monitor --- // --- Folder Monitor ---

View File

@ -14,14 +14,13 @@
- ✅ 3.3.10 — `npm audit fix` (non-breaking): 4 vulnerabilities geschlossen (16 → 12), nur Lock-file Update - ✅ 3.3.10 — `npm audit fix` (non-breaking): 4 vulnerabilities geschlossen (16 → 12), nur Lock-file Update
- ✅ 3.3.11 — Patch-Bumps `eslint 10.1→10.2`, `undici 7.24→7.25`, `ws 8.19→8.20` (semver-compatible) - ✅ 3.3.11 — Patch-Bumps `eslint 10.1→10.2`, `undici 7.24→7.25`, `ws 8.19→8.20` (semver-compatible)
- ✅ 3.3.12 — Race condition fix: `uploadManager = null` in batch-done clobberte einen frisch gespawnten Manager wenn user mid-await neuen batch startete (deep-audit finding HIGH-1) - ✅ 3.3.12 — Race condition fix: `uploadManager = null` in batch-done clobberte einen frisch gespawnten Manager wenn user mid-await neuen batch startete (deep-audit finding HIGH-1)
- ✅ 3.3.13 — `save-global-settings-sync` reportet jetzt `returnValue=false` bei Fehlern + debugLog statt silent swallow; TOCTOU bei .bak-Refresh in beiden Pfaden (main.js + lib/config-store.js _atomicWrite) entkoppelt: bak-Read-Failure failt nicht mehr den ganzen Save (deep-audit findings HIGH-2 + MED-4)
## Open items (priorisiert) ## Open items (priorisiert)
### Stabilität (neu aus deep-audit) ### Stabilität (neu aus deep-audit)
- [ ] `save-global-settings-sync` swallows write errors silently (main.js:1612-1628) — returns true even on disk-full / AV-lock. Should set returnValue=false + debugLog.
- [ ] `parseByseResult` / `parseDoodstreamResult` crash on null payload (lib/hosters.js:202, 214, 163, 491) — `JSON.parse('null')` returns null, then `payload.files` throws TypeError. Guard `if (!payload || typeof payload !== 'object') throw...` after JSON.parse. - [ ] `parseByseResult` / `parseDoodstreamResult` crash on null payload (lib/hosters.js:202, 214, 163, 491) — `JSON.parse('null')` returns null, then `payload.files` throws TypeError. Guard `if (!payload || typeof payload !== 'object') throw...` after JSON.parse.
- [ ] Cancellation latency in retry-loop's account-rotation block (lib/upload-manager.js:680-792) — re-check signal.aborted after each await. - [ ] Cancellation latency in retry-loop's account-rotation block (lib/upload-manager.js:680-792) — re-check signal.aborted after each await.
- [ ] TOCTOU on backup write in `_atomicWrite` (lib/config-store.js:254-258) — readFileSync after existsSync without try/catch; can fail if AV briefly locks file.
### Code-Qualität (deferred) ### Code-Qualität (deferred)
- [ ] removeFromQueueOnDone microtask-coalesce (3.3.1) — Microtask-Timing schwer zu testen ohne fake-timer setup - [ ] removeFromQueueOnDone microtask-coalesce (3.3.1) — Microtask-Timing schwer zu testen ohne fake-timer setup