Merge pull request #2771 from shijie152/fix/relay-key-atomic-settings

fix(cli): atomic settings writes and gate relay key regeneration in connect-url

packages/web/bin/lib/DOCUMENTATION.md

@@ -78,6 +78,18 @@ These modules hold reusable, non-presentational logic for commands.
 - `cli-paths.js`
   - Data, run, log, settings, tunnel profile, and managed-local config paths.
 
+- `cli-settings-accessors.js`
+  - Minimal settings.json read/write for CLI contexts that must not load the
+    full web settings runtime (`connect-url` relay identity resolution).
+  - Mirrors the settings runtime's guarantees so a CLI read-modify-write can
+    never corrupt shared state: atomic tmp+rename writes (no concurrent reader
+    in the running app can observe a torn file), a strict read that throws on
+    corrupt/unreadable payloads, and the same `0600` file mode.
+  - The strict read gates relay identity regeneration exactly like the server
+    runtime: a swallowed read failure can never mint a replacement signing or
+    encryption keypair, which would change `serverId` and orphan every paired
+    device and push binding.
+
 - `cli-process.js`
   - PID files, instance registry files, process identity checks, runtime metadata checks, and process termination helpers.
 

packages/web/bin/lib/cli-settings-accessors.js

@@ -0,0 +1,103 @@
+// Minimal settings.json access for CLI contexts (connect-url, pairing
+// candidate building) that must not load the full web settings runtime.
+//
+// The running app already treats settings.json as a shared store — the relay
+// identity, tunnels, notifications, the Electron main, and ssh-manager all
+// read-modify-write it. This accessor must therefore mirror the settings
+// runtime's guarantees or it will corrupt or regenerate shared state:
+//
+//   - ATOMIC writes (write tmp, rename into place). A plain writeFile can
+//     interleave with a concurrent reader in the running app; the reader sees
+//     a half-written file, its lenient read maps it to `{}`, and relay
+//     identity logic then mints a NEW serverId — orphaning every paired
+//     device. The tmp+rename below means no reader can ever observe a partial
+//     file.
+//
+//   - A STRICT read that THROWS on corrupt/unreadable payloads, gating relay
+//     identity regeneration. Only a genuinely missing file means "no
+//     settings"; any other failure (corrupt JSON, EACCES, transient I/O,
+//     non-object payload) must propagate so callers never confuse a broken
+//     read with first run and mint a replacement signing/encryption keypair.
+
+export const createSettingsAccessors = ({ fsPromises, path, dataDir, settingsFileName }) => {
+  const settingsPath = path.join(dataDir, settingsFileName);
+
+  const readSettingsFromDiskMigrated = async () => {
+    try {
+      return JSON.parse(await fsPromises.readFile(settingsPath, 'utf8'));
+    } catch {
+      return {};
+    }
+  };
+
+  const readSettingsStrict = async () => {
+    let raw;
+    try {
+      raw = await fsPromises.readFile(settingsPath, 'utf8');
+    } catch (error) {
+      if (error && typeof error === 'object' && error.code === 'ENOENT') {
+        return {};
+      }
+      throw error;
+    }
+    const parsed = JSON.parse(raw);
+    if (!parsed || typeof parsed !== 'object') {
+      throw new Error('Settings file is malformed (non-object payload)');
+    }
+    return parsed;
+  };
+
+  const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms));
+
+  const isTransientWindowsReplaceError = (error) => {
+    if (process.platform !== 'win32' || !error || typeof error !== 'object') {
+      return false;
+    }
+    return error.code === 'EPERM' || error.code === 'EACCES' || error.code === 'EBUSY';
+  };
+
+  const replaceFile = async (tmp, target) => {
+    const maxAttempts = process.platform === 'win32' ? 6 : 1;
+    let lastError = null;
+
+    for (let attempt = 1; attempt <= maxAttempts; attempt += 1) {
+      try {
+        await fsPromises.rename(tmp, target);
+        return;
+      } catch (error) {
+        lastError = error;
+        if (!isTransientWindowsReplaceError(error) || attempt === maxAttempts) {
+          break;
+        }
+        await sleep(25 * attempt);
+      }
+    }
+
+    if (!isTransientWindowsReplaceError(lastError)) {
+      throw lastError;
+    }
+
+    // Windows can transiently reject the atomic replace while another process
+    // briefly holds the target open. Fall back to copying the COMPLETE tmp file
+    // so persistence never wedges. Note: copyFile is NOT atomic — this is a
+    // last-resort path confined to Windows, matching the settings runtime's
+    // fallback, not a substitute for the atomic rename used everywhere else.
+    await fsPromises.copyFile(tmp, target);
+    await fsPromises.rm(tmp, { force: true });
+  };
+
+  const writeSettingsToDisk = async (settings) => {
+    await fsPromises.mkdir(path.dirname(settingsPath), { recursive: true });
+    const tmp = `${settingsPath}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`;
+    await fsPromises.writeFile(tmp, JSON.stringify(settings, null, 2), { encoding: 'utf8', mode: 0o600 });
+    if (process.platform !== 'win32') {
+      await fsPromises.chmod(tmp, 0o600);
+    }
+    await replaceFile(tmp, settingsPath);
+    if (process.platform !== 'win32') {
+      await fsPromises.chmod(settingsPath, 0o600);
+    }
+  };
+
+  return { readSettingsFromDiskMigrated, readSettingsStrict, writeSettingsToDisk };
+};

packages/web/bin/lib/cli-settings-accessors.test.js

@@ -0,0 +1,180 @@
+import { describe, expect, it } from 'vitest';
+import fs from 'fs';
+import os from 'os';
+import path from 'path';
+import crypto from 'crypto';
+
+import { createSettingsAccessors } from './cli-settings-accessors.js';
+import { createRelayIdentityRuntime } from '../../server/lib/relay/identity.js';
+
+const withTempDir = async (fn) => {
+  const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'oc-settings-accessors-'));
+  try {
+    return await fn(dir);
+  } finally {
+    fs.rmSync(dir, { recursive: true, force: true });
+  }
+};
+
+const makeAccessors = (dir, overrides = {}) =>
+  createSettingsAccessors({
+    fsPromises: fs.promises,
+    path,
+    dataDir: dir,
+    settingsFileName: 'settings.json',
+    ...overrides,
+  });
+
+// Wraps writeFile so each write lands in two chunks with a pause in between —
+// a stand-in for a large, slow write on a real disk (one open handle, so the
+// file grows from the prefix to the full payload). With a non-atomic writer a
+// concurrent reader deterministically catches the half-written file in that
+// window; with the atomic tmp+rename writer the target only ever changes via a
+// complete rename, so the window is never observable.
+const makeSlowWriteFs = () => {
+  const realFs = fs.promises;
+  const slowWriteFile = async (filePath, data) => {
+    const handle = await realFs.open(filePath, 'w');
+    try {
+      const half = Math.floor(data.length / 2);
+      await handle.writeFile(data.slice(0, half), 'utf8');
+      await new Promise((resolve) => setTimeout(resolve, 30));
+      await handle.writeFile(data.slice(half), 'utf8');
+    } finally {
+      await handle.close();
+    }
+  };
+  return { slowWriteFile, fsPromises: { ...realFs, writeFile: slowWriteFile } };
+};
+
+// Runs `writer` against filePath while a concurrent reader hammers it; returns
+// how many times the reader observed an unparseable (torn) payload. ENOENT
+// during the very first write is not a tear and is excluded.
+const countTornReads = async (filePath, writer, iterations) => {
+  const big = { theme: 'dark', filler: 'x'.repeat(4096) };
+  let torn = 0;
+  let stop = false;
+  const reader = (async () => {
+    while (!stop) {
+      try {
+        const parsed = JSON.parse(await fs.promises.readFile(filePath, 'utf8'));
+        if (parsed && typeof parsed === 'object') {
+          expect(parsed.theme).toBe('dark');
+        }
+      } catch (error) {
+        if (error?.code !== 'ENOENT') {
+          torn += 1;
+        }
+      }
+      await new Promise((resolve) => setTimeout(resolve, 0));
+    }
+  })();
+  for (let i = 0; i < iterations; i += 1) {
+    await writer({ ...big, n: i });
+  }
+  stop = true;
+  await reader;
+  return torn;
+};
+
+describe('cli settings accessors', () => {
+  it('persists the full object atomically and cleans up its tmp file', async () => {
+    await withTempDir(async (dir) => {
+      const accessors = makeAccessors(dir);
+      await accessors.writeSettingsToDisk({ theme: 'dark', count: 3 });
+
+      const raw = JSON.parse(fs.readFileSync(path.join(dir, 'settings.json'), 'utf8'));
+      expect(raw).toEqual({ theme: 'dark', count: 3 });
+
+      const leftovers = fs.readdirSync(dir).filter((name) => name.startsWith('settings.json.tmp-'));
+      expect(leftovers).toEqual([]);
+    });
+  });
+
+  it('atomic writes: concurrent readers never observe a torn file, even under slow writes', async () => {
+    await withTempDir(async (dir) => {
+      const { fsPromises } = makeSlowWriteFs();
+      const accessors = makeAccessors(dir, { fsPromises });
+      const filePath = path.join(dir, 'settings.json');
+
+      // Each write is chunked with a pause, yet the reader must never see a
+      // partial payload: the target only changes via a complete atomic rename.
+      const torn = await countTornReads(filePath, (settings) => accessors.writeSettingsToDisk(settings), 20);
+      expect(torn).toBe(0);
+
+      const leftovers = fs.readdirSync(dir).filter((name) => name.startsWith('settings.json.tmp-'));
+      expect(leftovers).toEqual([]);
+    });
+  });
+
+  it('demonstrates the protected failure mode: a naive direct writer tears under the same slow write', async () => {
+    await withTempDir(async (dir) => {
+      const { slowWriteFile } = makeSlowWriteFs();
+      const filePath = path.join(dir, 'settings.json');
+
+      // The old CLI accessor wrote straight to settings.json with writeFile.
+      // The same slow-write load therefore MUST produce torn reads — proving
+      // the concurrency test above can actually fail on the pre-fix writer.
+      const torn = await countTornReads(
+        filePath,
+        (settings) => slowWriteFile(filePath, JSON.stringify(settings)),
+        20,
+      );
+      expect(torn).toBeGreaterThan(0);
+    });
+  });
+
+  it('lenient read maps a corrupt file to {} for config lookup', async () => {
+    await withTempDir(async (dir) => {
+      const accessors = makeAccessors(dir);
+      fs.writeFileSync(path.join(dir, 'settings.json'), '{"unfinished": "trunc');
+      expect(await accessors.readSettingsFromDiskMigrated()).toEqual({});
+    });
+  });
+
+  it('strict read throws on a corrupt file instead of reporting "no settings"', async () => {
+    await withTempDir(async (dir) => {
+      const accessors = makeAccessors(dir);
+      fs.writeFileSync(path.join(dir, 'settings.json'), '{"unfinished": "trunc');
+      await expect(accessors.readSettingsStrict()).rejects.toThrow();
+    });
+  });
+
+  it('strict read throws on a non-object payload', async () => {
+    await withTempDir(async (dir) => {
+      const accessors = makeAccessors(dir);
+      fs.writeFileSync(path.join(dir, 'settings.json'), '"just a string"');
+      await expect(accessors.readSettingsStrict()).rejects.toThrow(/non-object payload/);
+    });
+  });
+
+  it('strict read treats only a genuinely missing file as no settings', async () => {
+    await withTempDir(async (dir) => {
+      const accessors = makeAccessors(dir);
+      expect(await accessors.readSettingsStrict()).toEqual({});
+    });
+  });
+
+  it('does not regenerate the relay identity off a corrupt settings file', async () => {
+    await withTempDir(async (dir) => {
+      const accessors = makeAccessors(dir);
+      fs.writeFileSync(
+        path.join(dir, 'settings.json'),
+        JSON.stringify({
+          relaySigningKey: {
+            privateJwk: crypto.generateKeyPairSync('ec', { namedCurve: 'P-256' }).privateKey.export({ format: 'jwk' }),
+            publicJwk: crypto.generateKeyPairSync('ec', { namedCurve: 'P-256' }).publicKey.export({ format: 'jwk' }),
+          },
+        }),
+      );
+      const identity = await createRelayIdentityRuntime({ crypto, ...accessors }).getRelayIdentity();
+      const serverIdBefore = identity.serverId;
+
+      // Corrupt the file, then ask for the identity again: the strict gate must
+      // make this FAIL rather than mint a replacement keypair.
+      fs.writeFileSync(path.join(dir, 'settings.json'), '{"relaySigningKey": {"unfinished');
+      await expect(createRelayIdentityRuntime({ crypto, ...accessors }).getRelayIdentity()).rejects.toThrow();
+      expect(serverIdBefore).toBeTruthy();
+    });
+  });
+});

packages/web/bin/lib/commands-connect-url.js

@@ -17,6 +17,7 @@ import { createClientPairingRuntime } from '../../server/lib/client-auth/pairing
 import { createRelayIdentityRuntime } from '../../server/lib/relay/identity.js';
 import { DEFAULT_RELAY_URL } from '../../server/lib/relay/service.js';
 import { bytesToBase64Url } from '../../server/lib/relay/e2ee.js';
+import { createSettingsAccessors as createSettingsAccessorsModule } from './cli-settings-accessors.js';
 import {
   intro as clackIntro,
   outro as clackOutro,
@@ -28,7 +29,6 @@ import {
 } from '../cli-output.js';
 
 const REMOTE_CLIENTS_FILE_NAME = 'remote-clients.json';
-const SETTINGS_FILE_NAME = 'settings.json';
 const PAIRING_SESSIONS_FILE_NAME = 'client-pairing-sessions.json';
 
 function isValidRelayUrl(value) {
@@ -55,20 +55,18 @@ function resolveRelayUrl(settings) {
 // Minimal settings.json read/write for the relay identity runtime. It reads the
 // whole object and writes it back with the relay keys added, so other settings
 // are preserved. Enough for the CLI without wiring the full settings runtime.
+//
+// Mirrors the settings runtime's guarantees: atomic writes (tmp + rename) so
+// concurrent readers in the running app never observe a half-written file, and
+// a STRICT reader gating relay identity regeneration so a swallowed read
+// failure can never mint a new serverId and orphan paired devices.
 function createSettingsAccessors() {
-  const settingsPath = path.join(getOpenChamberDataDir(), SETTINGS_FILE_NAME);
-  const readSettingsFromDiskMigrated = async () => {
-    try {
-      return JSON.parse(await fs.promises.readFile(settingsPath, 'utf8'));
-    } catch {
-      return {};
-    }
-  };
-  const writeSettingsToDisk = async (settings) => {
-    await fs.promises.mkdir(path.dirname(settingsPath), { recursive: true });
-    await fs.promises.writeFile(settingsPath, JSON.stringify(settings, null, 2), 'utf8');
-  };
-  return { readSettingsFromDiskMigrated, writeSettingsToDisk };
+  return createSettingsAccessorsModule({
+    fsPromises: fs.promises,
+    path,
+    dataDir: getOpenChamberDataDir(),
+    settingsFileName: 'settings.json',
+  });
 }
 
 // Resolves the instance's relay identity (serverId + encryption public key,