refactor: config file saving and debugging

Reduce the number of dependencies and the amount of code running in a
security sensitive context.

Instead of a deep comparison, we just compare the serialized versions
of the config files.

Signed-off-by: Kristóf Marussy <kristof@marussy.com>

9 files changed, 103 insertions, 94 deletions

diff --git a/packages/main/esbuild.config.js b/packages/main/esbuild.config.js
index ae8565d..dddda75 100644
--- a/packages/main/esbuild.config.js
+++ b/packages/main/esbuild.config.js
@@ -13,7 +13,11 @@ const thisDir = fileUrlToDirname(import.meta.url);
 const externalPackages = ['electron'];
 
 if (process.env.MODE !== 'development') {
-  externalPackages.push('electron-devtools-installer', 'source-map-support');
+  externalPackages.push(
+    'electron-devtools-installer',
+    'mkdirp',
+    'source-map-support',
+  );
 }
 
 const gitInfo = getRepoInfo();
diff --git a/packages/main/package.json b/packages/main/package.json
index fceb392..fc972ba 100644
--- a/packages/main/package.json
+++ b/packages/main/package.json
@@ -11,9 +11,7 @@
     "@sophie/service-shared": "workspace:*",
     "@sophie/shared": "workspace:*",
     "chalk": "^5.0.1",
-    "deep-equal": "^2.0.5",
     "electron": "^19.0.0-alpha.1",
-    "fs-extra": "^10.1.0",
     "i18next": "^21.6.16",
     "lodash-es": "^4.17.21",
     "loglevel": "^1.8.0",
@@ -21,15 +19,14 @@
     "mobx": "^6.5.0",
     "mobx-state-tree": "^5.1.3",
     "nanoid": "^3.3.3",
-    "os-name": "^5.0.1",
     "slug": "^5.3.0"
   },
   "devDependencies": {
     "@jest/globals": "^27.5.1",
     "@sophie/test-utils": "workspace:*",
-    "@types/deep-equal": "^1.0.1",
     "@types/electron-devtools-installer": "^2.2.2",
     "@types/lodash-es": "^4.17.6",
+    "@types/mkdirp": "^1",
     "@types/node": "^17.0.25",
     "@types/slug": "^5.0.3",
     "@types/source-map-support": "^0.5.4",
@@ -38,6 +35,7 @@
     "git-repo-info": "^2.1.1",
     "jest": "^27.5.1",
     "jest-mock": "^27.5.1",
+    "mkdirp": "^1.0.4",
     "source-map-support": "^0.5.21"
   }
 }
diff --git a/packages/main/src/index.ts b/packages/main/src/index.ts
index 29a8cca..78861c2 100644
--- a/packages/main/src/index.ts
+++ b/packages/main/src/index.ts
@@ -22,10 +22,11 @@
 import os from 'node:os';
 
 import { app } from 'electron';
-import { ensureDirSync } from 'fs-extra';
-import osName from 'os-name';
 
-import { enableStacktraceSourceMaps } from './infrastructure/electron/impl/devTools';
+import {
+  enableStacktraceSourceMaps,
+  ensureDevDataDir,
+} from './infrastructure/electron/impl/devTools';
 import electronShell from './infrastructure/electron/impl/electronShell';
 import initReactions from './initReactions';
 import MainStore from './stores/MainStore';
@@ -39,11 +40,7 @@ const log = getLogger('index');
 app.enableSandbox();
 
 if (isDevelopment) {
-  // Use alternative directory when debugging to avoid clobbering the main installation.
-  app.setPath('userData', `${app.getPath('userData')}-dev`);
-  ensureDirSync(app.getPath('userData'));
-
-  // Use source maps in stack traces.
+  ensureDevDataDir();
   enableStacktraceSourceMaps();
 }
 
@@ -67,7 +64,7 @@ app.setAboutPanelOptions({
     `Electron: ${process.versions.electron}`,
     `Chrome: ${process.versions.chrome}`,
     `Node.js: ${process.versions.node}`,
-    `Platform: ${osName()}`,
+    `Platform: ${os.platform()} ${os.release()}`,
     `Arch: ${os.arch()}`,
     `Build date: ${new Date(
       Number(import.meta.env.BUILD_DATE),
diff --git a/packages/main/src/infrastructure/config/ConfigRepository.ts b/packages/main/src/infrastructure/config/ConfigRepository.ts
index e00f5a0..67bffb0 100644
--- a/packages/main/src/infrastructure/config/ConfigRepository.ts
+++ b/packages/main/src/infrastructure/config/ConfigRepository.ts
@@ -18,17 +18,16 @@
  * SPDX-License-Identifier: AGPL-3.0-only
  */
 
-import type Config from '../../stores/config/Config';
 import type Disposer from '../../utils/Disposer';
 
 export type ReadConfigResult =
-  | { found: true; data: unknown }
+  | { found: true; contents: string }
   | { found: false };
 
 export default interface ConfigPersistence {
   readConfig(): Promise<ReadConfigResult>;
 
-  writeConfig(config: Config): Promise<void>;
+  writeConfig(contents: string): Promise<void>;
 
   watchConfig(callback: () => Promise<void>, throttleMs: number): Disposer;
 }
diff --git a/packages/main/src/infrastructure/config/impl/ConfigFile.ts b/packages/main/src/infrastructure/config/impl/ConfigFile.ts
index c4e6d22..8b110a2 100644
--- a/packages/main/src/infrastructure/config/impl/ConfigFile.ts
+++ b/packages/main/src/infrastructure/config/impl/ConfigFile.ts
@@ -24,7 +24,6 @@ import path from 'node:path';
 
 import { throttle } from 'lodash-es';
 
-import type Config from '../../../stores/config/Config';
 import type Disposer from '../../../utils/Disposer';
 import isErrno from '../../../utils/isErrno';
 import { getLogger } from '../../../utils/log';
@@ -48,9 +47,9 @@ export default class ConfigFile implements ConfigRepository {
   }
 
   async readConfig(): Promise<ReadConfigResult> {
-    let configStr: string;
+    let contents: string;
     try {
-      configStr = await readFile(this.configFilePath, 'utf8');
+      contents = await readFile(this.configFilePath, 'utf8');
     } catch (error) {
       if (isErrno(error, 'ENOENT')) {
         log.debug('Config file', this.configFilePath, 'was not found');
@@ -61,15 +60,14 @@ export default class ConfigFile implements ConfigRepository {
     log.info('Read config file', this.configFilePath);
     return {
       found: true,
-      data: JSON.parse(configStr),
+      contents,
     };
   }
 
-  async writeConfig(configSnapshot: Config): Promise<void> {
-    const configJson = JSON.stringify(configSnapshot, undefined, 2);
+  async writeConfig(contents: string): Promise<void> {
     this.writingConfig = true;
     try {
-      await writeFile(this.configFilePath, configJson, 'utf8');
+      await writeFile(this.configFilePath, contents, 'utf8');
       const { mtime } = await stat(this.configFilePath);
       log.trace('Config file', this.configFilePath, 'last written at', mtime);
       this.timeLastWritten = mtime;
diff --git a/packages/main/src/infrastructure/electron/impl/devTools.ts b/packages/main/src/infrastructure/electron/impl/devTools.ts
index 10f4545..6db88d1 100644
--- a/packages/main/src/infrastructure/electron/impl/devTools.ts
+++ b/packages/main/src/infrastructure/electron/impl/devTools.ts
@@ -18,34 +18,32 @@
  * SPDX-License-Identifier: AGPL-3.0-only
  */
 
-import type { BrowserWindow } from 'electron';
+import { app, type BrowserWindow } from 'electron';
+
+/* eslint-disable
+  import/no-extraneous-dependencies,
+  global-require,
+  @typescript-eslint/no-var-requires,
+  unicorn/prefer-module --
+  Hack to lazily require a CJS module from an ES module transpiled into a CJS module.
+*/
 
 /**
- * URL prefixes Sophie is allowed load in dev mode.
- *
- * In dev mode, in addition to the application itself,
- * Sophie must be able do download and load the devtools and related extensions,
- * so we have to make exceptions in the UI process request filter.
+ * Makes sure we use a separate data dir for development.
  */
-export const DEVMODE_ALLOWED_URL_PREFIXES = [
-  'chrome-extension:',
-  'devtools:',
-  'https://clients2.google.com/service/update2/crx',
-  'https://clients2.googleusercontent.com/crx',
-];
+export function ensureDevDataDir(): void {
+  // Use alternative directory when debugging to avoid clobbering the main installation.
+  app.setPath('userData', `${app.getPath('userData')}-dev`);
+  const userData = app.getPath('userData');
+  const mkdirp = require('mkdirp') as typeof import('mkdirp');
+  mkdirp.sync(userData);
+}
 
 /**
  * Enables using source maps for node stack traces.
  */
 export function enableStacktraceSourceMaps(): void {
   const sourceMapSupport =
-    /* eslint-disable-next-line
-      import/no-extraneous-dependencies,
-      global-require,
-      @typescript-eslint/no-var-requires,
-      unicorn/prefer-module --
-      Hack to lazily require a CJS module from an ES module transpiled into a CJS module.
-    */
     require('source-map-support') as typeof import('source-map-support');
   sourceMapSupport.install();
 }
@@ -61,13 +59,6 @@ export async function installDevToolsExtensions(): Promise<void> {
     default: installExtension,
     REACT_DEVELOPER_TOOLS,
     REDUX_DEVTOOLS,
-    /* eslint-disable-next-line
-      import/no-extraneous-dependencies,
-      global-require,
-      @typescript-eslint/no-var-requires,
-      unicorn/prefer-module --
-      Hack to lazily require a CJS module from an ES module transpiled into a CJS module.
-    */
   } = require('electron-devtools-installer') as typeof import('electron-devtools-installer');
   await installExtension([REACT_DEVELOPER_TOOLS, REDUX_DEVTOOLS], {
     forceDownload: false,
diff --git a/packages/main/src/infrastructure/electron/impl/hardenSession.ts b/packages/main/src/infrastructure/electron/impl/hardenSession.ts
index 10b694a..53675a7 100644
--- a/packages/main/src/infrastructure/electron/impl/hardenSession.ts
+++ b/packages/main/src/infrastructure/electron/impl/hardenSession.ts
@@ -25,7 +25,19 @@ import type { Session } from 'electron';
 import { getLogger } from '../../../utils/log';
 import type Resources from '../../resources/Resources';
 
-import { DEVMODE_ALLOWED_URL_PREFIXES } from './devTools';
+/**
+ * URL prefixes Sophie is allowed load in dev mode.
+ *
+ * In dev mode, in addition to the application itself,
+ * Sophie must be able do download and load the devtools and related extensions,
+ * so we have to make exceptions in the UI process request filter.
+ */
+export const DEVMODE_ALLOWED_URL_PREFIXES = [
+  'chrome-extension:',
+  'devtools:',
+  'https://clients2.google.com/service/update2/crx',
+  'https://clients2.googleusercontent.com/crx',
+];
 
 const log = getLogger('hardenSession');
 
diff --git a/packages/main/src/reactions/__tests__/synchronizeConfig.spec.ts b/packages/main/src/reactions/__tests__/synchronizeConfig.spec.ts
index b5013ea..d3338d0 100644
--- a/packages/main/src/reactions/__tests__/synchronizeConfig.spec.ts
+++ b/packages/main/src/reactions/__tests__/synchronizeConfig.spec.ts
@@ -25,7 +25,7 @@ import type ConfigRepository from '../../infrastructure/config/ConfigRepository'
 import SharedStore from '../../stores/SharedStore';
 import type Disposer from '../../utils/Disposer';
 import { silenceLogger } from '../../utils/log';
-import synchronizeConfig from '../synchronizeConfig';
+import synchronizeConfig, { serializeConfig } from '../synchronizeConfig';
 
 let store: SharedStore;
 const repository: ConfigRepository = {
@@ -70,11 +70,11 @@ describe('when synchronizeializing', () => {
     beforeEach(() => {
       mocked(repository.readConfig).mockResolvedValueOnce({
         found: true,
-        data: {
+        contents: serializeConfig({
           // Use a default empty config file to not trigger config rewrite.
           ...store.config,
           themeSource: 'dark',
-        },
+        }),
       });
     });
 
@@ -97,12 +97,15 @@ describe('when synchronizeializing', () => {
   it('should update the config file if new details are added during read', async () => {
     mocked(repository.readConfig).mockResolvedValueOnce({
       found: true,
-      data: {
-        themeSource: 'light',
-        profile: {
-          name: 'Test profile',
-        },
-      },
+      contents: `{
+  "themeSource": "light",
+  "profiles": [
+    {
+      "name": "Test profile"
+    }
+  ]
+}
+`,
     });
     await synchronizeConfig(store, repository);
     expect(repository.writeConfig).toHaveBeenCalledTimes(1);
@@ -111,9 +114,10 @@ describe('when synchronizeializing', () => {
   it('should not apply an invalid config file but should not overwrite it', async () => {
     mocked(repository.readConfig).mockResolvedValueOnce({
       found: true,
-      data: {
-        themeSource: -1,
-      },
+      contents: `{
+  "themeSource": -1
+}
+`,
     });
     await synchronizeConfig(store, repository);
     expect(store.settings.themeSource).not.toBe(-1);
@@ -136,7 +140,7 @@ describe('when it has loaded the config', () => {
   beforeEach(async () => {
     mocked(repository.readConfig).mockResolvedValueOnce({
       found: true,
-      data: store.config,
+      contents: serializeConfig(store.config),
     });
     mocked(repository.watchConfig).mockReturnValueOnce(watcherDisposer);
     sutDisposer = await synchronizeConfig(store, repository, throttleMs);
@@ -163,11 +167,11 @@ describe('when it has loaded the config', () => {
   it('should read the config file when it has changed', async () => {
     mocked(repository.readConfig).mockResolvedValueOnce({
       found: true,
-      data: {
+      contents: serializeConfig({
         // Use a default empty config file to not trigger config rewrite.
         ...store.config,
         themeSource: 'dark',
-      },
+      }),
     });
     await configChangedCallback();
     // Do not write back the changes we have just read.
@@ -178,12 +182,15 @@ describe('when it has loaded the config', () => {
   it('should update the config file if new details are added', async () => {
     mocked(repository.readConfig).mockResolvedValueOnce({
       found: true,
-      data: {
-        themeSource: 'light',
-        profile: {
-          name: 'Test profile',
-        },
-      },
+      contents: `{
+  "themeSource": "light",
+  "profiles": [
+    {
+      "name": "Test profile"
+    }
+  ]
+}
+`,
     });
     await configChangedCallback();
     expect(repository.writeConfig).toHaveBeenCalledTimes(1);
@@ -192,9 +199,10 @@ describe('when it has loaded the config', () => {
   it('should not apply an invalid config file when it has changed but should not overwrite it', async () => {
     mocked(repository.readConfig).mockResolvedValueOnce({
       found: true,
-      data: {
-        themeSource: -1,
-      },
+      contents: `{
+  "themeSource": -1
+}
+`,
     });
     await configChangedCallback();
     expect(store.settings.themeSource).not.toBe(-1);
diff --git a/packages/main/src/reactions/synchronizeConfig.ts b/packages/main/src/reactions/synchronizeConfig.ts
index 7e366e2..247c2e2 100644
--- a/packages/main/src/reactions/synchronizeConfig.ts
+++ b/packages/main/src/reactions/synchronizeConfig.ts
@@ -18,7 +18,6 @@
  * SPDX-License-Identifier: AGPL-3.0-only
  */
 
-import deepEqual from 'deep-equal';
 import { debounce } from 'lodash-es';
 import { reaction } from 'mobx';
 
@@ -32,36 +31,38 @@ const DEFAULT_CONFIG_DEBOUNCE_TIME_MS = 1000;
 
 const log = getLogger('synchronizeConfig');
 
+export function serializeConfig(config: Config): string {
+  return `${JSON.stringify(config, undefined, 2)}\n`;
+}
+
 export default async function synchronizeConfig(
   sharedStore: SharedStore,
   repository: ConfigRepository,
   debounceTime: number = DEFAULT_CONFIG_DEBOUNCE_TIME_MS,
 ): Promise<Disposer> {
-  let lastConfigOnDisk: Config | undefined;
+  let lastConfigOnDisk: string | undefined;
 
-  async function writeConfig(): Promise<void> {
-    const { config } = sharedStore;
-    await repository.writeConfig(config);
-    lastConfigOnDisk = config;
+  async function writeConfig(serializedConfig: string): Promise<void> {
+    await repository.writeConfig(serializedConfig);
+    lastConfigOnDisk = serializedConfig;
   }
 
   async function readConfig(): Promise<boolean> {
     const result = await repository.readConfig();
     if (result.found) {
+      const { contents } = result;
       try {
         // This cast is unsound if the config file is invalid,
         // but we'll throw an error in the end anyways.
-        sharedStore.loadConfig(result.data as Config);
+        const data = JSON.parse(contents) as Config;
+        sharedStore.loadConfig(data);
       } catch (error) {
-        log.error('Failed to apply config snapshot', result.data, error);
+        log.error('Failed to apply config snapshot', contents, error);
         return true;
       }
-      lastConfigOnDisk = sharedStore.config;
-      // We can't use `comparer.structural` from `mobx`, because
-      // it handles missing values and `undefined` values differently,
-      // but JSON is unable to distinguish them.
-      if (!deepEqual(result.data, lastConfigOnDisk, { strict: true })) {
-        await writeConfig();
+      lastConfigOnDisk = serializeConfig(sharedStore.config);
+      if (contents !== lastConfigOnDisk) {
+        await writeConfig(lastConfigOnDisk);
       }
     }
     return result.found;
@@ -69,16 +70,17 @@ export default async function synchronizeConfig(
 
   if (!(await readConfig())) {
     log.info('Config file was not found');
-    await writeConfig();
+    const serializedConfig = serializeConfig(sharedStore.config);
+    await writeConfig(serializedConfig);
     log.info('Created config file');
   }
 
   const disposeReaction = reaction(
     () => sharedStore.config,
-    debounce((config) => {
-      // We can compare snapshots by reference, since it is only recreated on store changes.
-      if (!deepEqual(config, lastConfigOnDisk, { strict: true })) {
-        writeConfig().catch((error) => {
+    debounce(() => {
+      const serializedConfig = serializeConfig(sharedStore.config);
+      if (serializedConfig !== lastConfigOnDisk) {
+        writeConfig(serializedConfig).catch((error) => {
           log.error('Failed to write config on config change', error);
         });
       }