From a6958f14184526477eab0b8924c83556b72213a9 Mon Sep 17 00:00:00 2001 From: Administrator Date: Tue, 28 Apr 2026 09:40:08 +0200 Subject: [PATCH] fix(persist): stop swallowing save errors + decouple .bak refresh from save MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- lib/config-store.js | 16 +++++++++++----- main.js | 25 +++++++++++++++++++------ tasks/todo.md | 3 +-- 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/lib/config-store.js b/lib/config-store.js index c271f58..78b7c52 100644 --- a/lib/config-store.js +++ b/lib/config-store.js @@ -251,12 +251,18 @@ class ConfigStore { fs.writeFile(tmpPath, data, 'utf-8', (err) => { if (err) return reject(err); try { - if (fs.existsSync(this.filePath)) { - const existing = fs.readFileSync(this.filePath, 'utf-8'); - if (existing && existing.trim().length > 2) { - fs.writeFileSync(backupPath, existing, 'utf-8'); + // 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 { + if (fs.existsSync(this.filePath)) { + const existing = fs.readFileSync(this.filePath, 'utf-8'); + if (existing && existing.trim().length > 2) { + fs.writeFileSync(backupPath, existing, 'utf-8'); + } } - } + } catch {} fs.renameSync(tmpPath, this.filePath); } catch (e) { return reject(e); } resolve(); diff --git a/main.js b/main.js index fccc987..d0991d9 100644 --- a/main.js +++ b/main.js @@ -1616,7 +1616,10 @@ ipcMain.handle('save-global-settings', async (_event, globalSettings) => { }); // 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) => { try { const current = configStore.load(); @@ -1626,14 +1629,24 @@ ipcMain.on('save-global-settings-sync', (event, globalSettings) => { const backupPath = configStore.filePath + '.bak'; fs.writeFileSync(tmpPath, data, 'utf-8'); if (fs.existsSync(configStore.filePath)) { - const existing = fs.readFileSync(configStore.filePath, 'utf-8'); - if (existing && existing.trim().length > 2) { - fs.writeFileSync(backupPath, existing, 'utf-8'); + // 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'); + if (existing && existing.trim().length > 2) { + fs.writeFileSync(backupPath, existing, 'utf-8'); + } + } catch (bakErr) { + debugLog(`save-global-settings-sync: backup read/write skipped: ${bakErr.message}`); } } 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 --- diff --git a/tasks/todo.md b/tasks/todo.md index 9e05dbe..b02af1a 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -14,14 +14,13 @@ - ✅ 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.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) ### 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. - [ ] 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) - [ ] removeFromQueueOnDone microtask-coalesce (3.3.1) — Microtask-Timing schwer zu testen ohne fake-timer setup