fix(cli): atomic settings writes and gate relay key regeneration in connect-url
The CLI's settings accessors wrote settings.json directly with writeFile and
read it leniently, with no strict-reader gate on relay identity. Running
'openchamber connect-url' while the desktop app is up could:
- tear the file for a concurrent reader in the app, tripping the relay
service's read and mapping it to {} (first-run);
- then regenerate the relay signing/encryption keys, changing serverId and
orphaning every paired device and push binding.
Move the accessors into a dedicated module that mirrors the settings
runtime's guarantees: atomic tmp+rename writes (with the Windows fallback)
so no reader can observe a partial file, and a strict reader that throws on
corrupt/unreadable payloads so identity regeneration is gated exactly like
the server runtime. Wire the strict reader into the CLI relay identity path.
Adds unit tests covering atomic writes under concurrent readers, strict-read
behavior, and that a corrupt settings file makes getRelayIdentity fail
instead of minting a replacement keypair.
This commit is contained in:
@@ -0,0 +1,101 @@
|
||||
// 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. Copy the COMPLETE tmp file into place so
|
||||
// persistence never wedges; a reader can still never see partial content.
|
||||
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 };
|
||||
};
|
||||
@@ -0,0 +1,127 @@
|
||||
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) =>
|
||||
createSettingsAccessors({ fsPromises: fs.promises, path, dataDir: dir, settingsFileName: 'settings.json' });
|
||||
|
||||
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('never leaves a partial file observable by a concurrent reader during writes', async () => {
|
||||
await withTempDir(async (dir) => {
|
||||
const accessors = makeAccessors(dir);
|
||||
const filePath = path.join(dir, 'settings.json');
|
||||
|
||||
// Hammer reads concurrently with writes; every observed payload must be a
|
||||
// complete, parseable object (the old plain writeFile could surface a
|
||||
// torn file mid-rename, which is what tripped the relay identity logic).
|
||||
const stop = { value: false };
|
||||
const reader = (async () => {
|
||||
while (!stop.value) {
|
||||
try {
|
||||
const parsed = JSON.parse(await fs.promises.readFile(filePath, 'utf8'));
|
||||
if (parsed && typeof parsed === 'object') {
|
||||
// A complete object is always fine; anything else would be a tear.
|
||||
expect(parsed.theme).toBe('dark');
|
||||
}
|
||||
} catch {
|
||||
// ENOENT during the very first write is acceptable.
|
||||
}
|
||||
await new Promise((resolve) => setTimeout(resolve, 0));
|
||||
}
|
||||
})();
|
||||
|
||||
const big = { theme: 'dark', filler: 'x'.repeat(4096) };
|
||||
await Promise.all(
|
||||
Array.from({ length: 50 }, (_, i) =>
|
||||
accessors.writeSettingsToDisk({ ...big, n: i }).catch(() => {}),
|
||||
),
|
||||
);
|
||||
stop.value = true;
|
||||
await reader;
|
||||
|
||||
const final = JSON.parse(fs.readFileSync(filePath, 'utf8'));
|
||||
expect(final.theme).toBe('dark');
|
||||
});
|
||||
});
|
||||
|
||||
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();
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user