From 453d980a41df3e203318cd97c4e795d26c3e4058 Mon Sep 17 00:00:00 2001 From: aakhter Date: Tue, 21 Apr 2026 09:03:30 -0400 Subject: [PATCH] fix(electron): harden simple store writes (#7297) * fix: harden electron simple store writes * fix(electron): harden simple store writes --------- Co-authored-by: Aamer Akhter --- electron/simple-store.test.cjs | 128 +++++++++++++++++++++++++++++++++ electron/simple-store.ts | 51 +++++++++++-- 2 files changed, 172 insertions(+), 7 deletions(-) create mode 100644 electron/simple-store.test.cjs diff --git a/electron/simple-store.test.cjs b/electron/simple-store.test.cjs new file mode 100644 index 0000000000..195119c440 --- /dev/null +++ b/electron/simple-store.test.cjs @@ -0,0 +1,128 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const os = require('node:os'); +const path = require('node:path'); +const { promises: fs } = require('node:fs'); +const fsModule = require('node:fs'); +const Module = require('node:module'); + +require('ts-node/register/transpile-only'); + +const originalModuleLoad = Module._load; +const originalWriteFile = fsModule.promises.writeFile; + +let userDataDir; +let logCalls = []; +let errorCalls = []; + +const simpleStoreModulePath = path.resolve(__dirname, 'simple-store.ts'); +const getStorePath = () => path.join(userDataDir, 'simpleSettings'); + +const installMocks = () => { + Module._load = function patchedLoad(request, parent, isMain) { + if (request === 'electron') { + return { + app: { + getPath: (key) => { + assert.equal(key, 'userData'); + return userDataDir; + }, + }, + }; + } + if (request === 'electron-log/main') { + return { + log: (...args) => logCalls.push(args), + error: (...args) => errorCalls.push(args), + }; + } + return originalModuleLoad.call(this, request, parent, isMain); + }; +}; + +const resetModule = () => { + delete require.cache[simpleStoreModulePath]; +}; + +const loadSimpleStoreModule = () => { + resetModule(); + return require(simpleStoreModulePath); +}; + +test.beforeEach(async () => { + userDataDir = await fs.mkdtemp(path.join(os.tmpdir(), 'sp-simple-store-')); + logCalls = []; + errorCalls = []; + fsModule.promises.writeFile = originalWriteFile; + installMocks(); +}); + +test.afterEach(async () => { + Module._load = originalModuleLoad; + fsModule.promises.writeFile = originalWriteFile; + resetModule(); + await fs.rm(userDataDir, { recursive: true, force: true }); +}); + +test('loadSimpleStoreAll quarantines corrupt JSON files', async () => { + const storePath = getStorePath(); + await fs.writeFile(storePath, '{"broken"', 'utf8'); + + const { loadSimpleStoreAll } = loadSimpleStoreModule(); + const data = await loadSimpleStoreAll(); + + assert.deepEqual(data, {}); + + const dirEntries = await fs.readdir(userDataDir); + const quarantined = dirEntries.find((entry) => entry.startsWith('simpleSettings.corrupt-')); + assert.ok(quarantined, 'expected corrupt file to be quarantined'); + await assert.rejects(fs.access(storePath)); + assert.ok( + errorCalls.some(([message]) => + String(message).includes('Failed to parse simple store JSON'), + ), + ); +}); + +test('saveSimpleStore replaces legacy directories with a file', async () => { + const storePath = getStorePath(); + await fs.mkdir(storePath, { recursive: true }); + + const { saveSimpleStore, loadSimpleStoreAll } = loadSimpleStoreModule(); + await saveSimpleStore('main', { theme: 'dark' }); + + const stat = await fs.stat(storePath); + assert.equal(stat.isFile(), true); + assert.deepEqual(await loadSimpleStoreAll(), { main: { theme: 'dark' } }); +}); + +test('saveSimpleStore serializes concurrent writes so keys are merged', async () => { + const { saveSimpleStore, loadSimpleStoreAll } = loadSimpleStoreModule(); + + let unblockFirstWrite; + const firstWriteBlocked = new Promise((resolve) => { + unblockFirstWrite = resolve; + }); + let firstWriteSeen = false; + + fsModule.promises.writeFile = async (...args) => { + const [target] = args; + if (!firstWriteSeen && String(target).includes('.tmp')) { + firstWriteSeen = true; + await firstWriteBlocked; + } + return originalWriteFile.apply(fsModule.promises, args); + }; + + const firstSave = saveSimpleStore('alpha', { ok: true }); + await new Promise((resolve) => setImmediate(resolve)); + const secondSave = saveSimpleStore('beta', { ok: true }); + + unblockFirstWrite(); + await Promise.all([firstSave, secondSave]); + + assert.deepEqual(await loadSimpleStoreAll(), { + alpha: { ok: true }, + beta: { ok: true }, + }); +}); diff --git a/electron/simple-store.ts b/electron/simple-store.ts index 26ad446ce4..003d8fc509 100644 --- a/electron/simple-store.ts +++ b/electron/simple-store.ts @@ -9,21 +9,43 @@ const getDataPath = (): string => path.join(app.getPath('userData'), 'simpleSett type SimpleStoreData = { [key: string]: unknown }; -export const saveSimpleStore = async (dataKey = 'main', data: unknown): Promise => { - const prevData = await loadSimpleStoreAll(); +let _saveQueue: Promise = Promise.resolve(); + +const _getTmpDataPath = (): string => `${getDataPath()}.${process.pid}.${Date.now()}.tmp`; + +const _quarantineCorruptStore = async (): Promise => { const dataPath = getDataPath(); - const json = JSON.stringify({ ...prevData, [dataKey]: data }); + const corruptPath = `${dataPath}.corrupt-${Date.now()}`; + try { + await fs.rename(dataPath, corruptPath); + log(`Quarantined corrupt simpleSettings file to ${corruptPath}`); + } catch (renameErr) { + error('Failed to quarantine corrupt simple store:', renameErr); + } +}; + +const _writeStoreAtomically = async (json: string): Promise => { + const dataPath = getDataPath(); + const tmpPath = _getTmpDataPath(); try { - await fs.writeFile(dataPath, json, { encoding: 'utf8' }); + await fs.writeFile(tmpPath, json, { encoding: 'utf8' }); + await fs.rename(tmpPath, dataPath); } catch (e: unknown) { const nodeErr = e as NodeJS.ErrnoException; + try { + await fs.rm(tmpPath, { force: true }); + } catch { + // Best effort cleanup only. + } + if (nodeErr.code === 'EISDIR') { // In older app versions simpleSettings was stored as a directory. // Remove the legacy directory so we can write the file. log('simpleSettings is a directory, removing for file-based storage'); await fs.rm(dataPath, { recursive: true }); - await fs.writeFile(dataPath, json, { encoding: 'utf8' }); + await fs.writeFile(tmpPath, json, { encoding: 'utf8' }); + await fs.rename(tmpPath, dataPath); } else { error('Failed to save simple store:', e); throw e; @@ -31,6 +53,17 @@ export const saveSimpleStore = async (dataKey = 'main', data: unknown): Promise< } }; +export const saveSimpleStore = async (dataKey = 'main', data: unknown): Promise => { + const runSave = async (): Promise => { + const prevData = await loadSimpleStoreAll(); + const json = JSON.stringify({ ...prevData, [dataKey]: data }); + await _writeStoreAtomically(json); + }; + + _saveQueue = _saveQueue.then(runSave, runSave); + return _saveQueue; +}; + export const loadSimpleStoreAll = async (): Promise => { try { const data = await fs.readFile(getDataPath(), { encoding: 'utf8' }); @@ -48,8 +81,12 @@ export const loadSimpleStoreAll = async (): Promise => { } catch (renameErr) { error('Failed to rename legacy simpleSettings directory:', renameErr); } - } else if (nodeErr.code !== 'ENOENT') { - // ENOENT is expected on first run — only log unexpected errors + } else if (nodeErr.code === 'ENOENT') { + // ENOENT is expected on first run. + } else if (e instanceof SyntaxError) { + error('Failed to parse simple store JSON, quarantining corrupt file:', e); + await _quarantineCorruptStore(); + } else { error('Failed to load simple store:', e); } return {};