diff --git a/scripts/app-layout.ts b/scripts/app-layout.ts index afb7de67..a70d0c8d 100644 --- a/scripts/app-layout.ts +++ b/scripts/app-layout.ts @@ -5,6 +5,7 @@ import path from 'node:path' import { rendererContentSecurityPolicy } from '../src/renderer-security' export const runtimeModuleNames = [ + 'atomic-json', 'about-panel', 'agent-guidance', 'agent-reviewer-guidance', diff --git a/src/atomic-json.ts b/src/atomic-json.ts new file mode 100644 index 00000000..598e4114 --- /dev/null +++ b/src/atomic-json.ts @@ -0,0 +1,40 @@ +import { randomBytes } from 'node:crypto' +import fs from 'node:fs/promises' +import path from 'node:path' + +export interface AtomicJsonReplacementOptions { + platform?: NodeJS.Platform +} + +function errorCode(error: unknown): unknown { + return error !== null && typeof error === 'object' + ? Reflect.get(error, 'code') + : null +} + +export async function replaceJsonFile( + filePath: string, + value: unknown, + { platform = process.platform }: AtomicJsonReplacementOptions = {} +): Promise { + const contents = `${JSON.stringify(value, null, 2)}\n` + const temporaryPath = path.join( + path.dirname(filePath), + `.${path.basename(filePath)}-${String(process.pid)}-${randomBytes(6).toString('hex')}.tmp` + ) + + try { + await fs.writeFile(temporaryPath, contents, { + encoding: 'utf8', + flag: 'wx', + flush: true, + mode: 0o600 + }) + if (platform !== 'win32') await fs.chmod(temporaryPath, 0o600) + await fs.rename(temporaryPath, filePath) + } finally { + await fs.unlink(temporaryPath).catch((error: unknown) => { + if (errorCode(error) !== 'ENOENT') throw error + }) + } +} diff --git a/src/review-store.ts b/src/review-store.ts index 8fbab073..3e440be9 100644 --- a/src/review-store.ts +++ b/src/review-store.ts @@ -2,7 +2,7 @@ import { createHash, randomBytes } from 'node:crypto' import fs from 'node:fs/promises' import path from 'node:path' import { isDeepStrictEqual } from 'node:util' - +import { replaceJsonFile } from './atomic-json' import { MAXIMUM_ATTACHMENT_BYTES } from './attachment-limits' import { guidance } from './agent-guidance' import { reviewerGuidance } from './agent-reviewer-guidance' @@ -1690,23 +1690,7 @@ export class ReviewStore { } async writeFile(filePath: string, artifact: unknown): Promise { - const temporaryPath = path.join( - path.dirname(filePath), - `.review-${String(process.pid)}-${randomBytes(6).toString('hex')}.tmp` - ) - - try { - await fs.writeFile( - temporaryPath, - `${JSON.stringify(artifact, null, 2)}\n`, - { encoding: 'utf8', flag: 'wx', flush: true } - ) - await fs.rename(temporaryPath, filePath) - } finally { - await fs.unlink(temporaryPath).catch((error: unknown) => { - if (errorCode(error) !== 'ENOENT') throw error - }) - } + await replaceJsonFile(filePath, artifact) } serialize(key: string, operation: () => Promise): Promise { diff --git a/src/service-endpoint.ts b/src/service-endpoint.ts index c800d37c..bc9b569c 100644 --- a/src/service-endpoint.ts +++ b/src/service-endpoint.ts @@ -3,6 +3,8 @@ import fs from 'node:fs/promises' import os from 'node:os' import path from 'node:path' +import { replaceJsonFile } from './atomic-json' + export const CAPABILITY_TOKEN_PATTERN = /^[A-Za-z0-9_-]{43}$/ export const SERVICE_INSTANCE_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i @@ -128,28 +130,7 @@ async function writePrivateJson( value: unknown, platform: NodeJS.Platform ): Promise { - const temporaryPath = path.join( - path.dirname(filePath), - `.${path.basename(filePath)}-${String(process.pid)}-${randomBytes(6).toString('hex')}.tmp` - ) - try { - await fs.writeFile(temporaryPath, `${JSON.stringify(value, null, 2)}\n`, { - encoding: 'utf8', - flag: 'wx', - flush: true, - mode: 0o600 - }) - if (platform !== 'win32') await fs.chmod(temporaryPath, 0o600) - await fs.rename(temporaryPath, filePath) - } finally { - await fs.unlink(temporaryPath).catch((error: unknown) => { - if ( - error === null || - typeof error !== 'object' || - Reflect.get(error, 'code') !== 'ENOENT' - ) throw error - }) - } + await replaceJsonFile(filePath, value, { platform }) } export interface PublishServiceConnectionOptions { diff --git a/src/settings-store.ts b/src/settings-store.ts index 4fa78bf3..019e3b40 100644 --- a/src/settings-store.ts +++ b/src/settings-store.ts @@ -1,6 +1,7 @@ import fs from 'node:fs/promises' import path from 'node:path' +import { replaceJsonFile } from './atomic-json' import { DEFAULT_SETTINGS, normalizeSettings, updateSettings } from './settings' interface SettingsReadResult { @@ -18,7 +19,6 @@ export class SettingsStore { settings: MarkoverSettings private readonly initialSettings: MarkoverSettings private writer: Promise - private writeSequence: number constructor(filePath: string, initialSettings: unknown = DEFAULT_SETTINGS) { this.filePath = filePath @@ -26,7 +26,6 @@ export class SettingsStore { this.initialSettings = normalizeSettings(initialSettings) this.settings = { ...this.initialSettings } this.writer = Promise.resolve() - this.writeSequence = 0 } private async readResult(): Promise { @@ -67,25 +66,12 @@ export class SettingsStore { } async update(patch: unknown): Promise { - const sequence = ++this.writeSequence const write = this.writer.catch(() => undefined).then(async () => { await fs.mkdir(path.dirname(this.filePath), { recursive: true }) - const temporaryPath = `${this.filePath}.${String(process.pid)}.${String(sequence)}.tmp` - try { - const snapshot = updateSettings(this.settings, patch) - await fs.writeFile( - temporaryPath, - `${JSON.stringify(snapshot, null, 2)}\n`, - 'utf8' - ) - await fs.rename(temporaryPath, this.filePath) - this.settings = snapshot - return { ...snapshot } - } finally { - await fs.unlink(temporaryPath).catch((error: unknown) => { - if (errorProperty(error, 'code') !== 'ENOENT') throw error - }) - } + const snapshot = updateSettings(this.settings, patch) + await replaceJsonFile(this.filePath, snapshot) + this.settings = snapshot + return { ...snapshot } }) this.writer = write.then(() => undefined, () => undefined) return write diff --git a/src/workspace-store.ts b/src/workspace-store.ts index 10f8ecd2..00e969b2 100644 --- a/src/workspace-store.ts +++ b/src/workspace-store.ts @@ -1,6 +1,7 @@ import fs from 'node:fs/promises' import path from 'node:path' +import { replaceJsonFile } from './atomic-json' import { cloneWorkspaceState, defaultWorkspaceState, @@ -23,7 +24,6 @@ export class WorkspaceStore { private writer: Promise = Promise.resolve() private latestWrite: Promise = Promise.resolve() private latestSnapshot: MarkoverWorkspaceState | null = null - private writeSequence = 0 constructor(filePath: string) { this.filePath = filePath @@ -66,23 +66,10 @@ export class WorkspaceStore { } private enqueueWrite(snapshot: MarkoverWorkspaceState): Promise { - const sequence = ++this.writeSequence const write = this.writer.catch(() => undefined).then(async () => { await fs.mkdir(path.dirname(this.filePath), { recursive: true }) - const temporaryPath = `${this.filePath}.${String(process.pid)}.${String(sequence)}.tmp` - try { - await fs.writeFile( - temporaryPath, - `${JSON.stringify(snapshot, null, 2)}\n`, - { encoding: 'utf8', mode: 0o600 } - ) - await fs.rename(temporaryPath, this.filePath) - this.state = cloneWorkspaceState(snapshot) - } finally { - await fs.unlink(temporaryPath).catch((error: unknown) => { - if (errorProperty(error, 'code') !== 'ENOENT') throw error - }) - } + await replaceJsonFile(this.filePath, snapshot) + this.state = cloneWorkspaceState(snapshot) }) this.writer = write.then(() => undefined, () => undefined) this.latestWrite = write diff --git a/test/atomic-json.test.ts b/test/atomic-json.test.ts new file mode 100644 index 00000000..bed79343 --- /dev/null +++ b/test/atomic-json.test.ts @@ -0,0 +1,46 @@ +import assert from 'node:assert/strict' +import fs from 'node:fs/promises' +import os from 'node:os' +import path from 'node:path' +import test from 'node:test' + +import { replaceJsonFile } from '../src/atomic-json' + +test('atomically replaces private pretty-printed JSON without temporary files', async (t) => { + const directory = await fs.mkdtemp(path.join(os.tmpdir(), 'markover-atomic-json-')) + t.after(() => fs.rm(directory, { recursive: true, force: true })) + const filePath = path.join(directory, 'state.json') + await fs.writeFile(filePath, '{}\n', { mode: 0o666 }) + + await replaceJsonFile(filePath, { current: true }) + + assert.equal(await fs.readFile(filePath, 'utf8'), '{\n "current": true\n}\n') + assert.deepEqual(await fs.readdir(directory), ['state.json']) + if (process.platform !== 'win32') { + assert.equal((await fs.stat(filePath)).mode & 0o777, 0o600) + } +}) + +test('preserves the destination when JSON serialization fails before replacement', async (t) => { + const directory = await fs.mkdtemp(path.join(os.tmpdir(), 'markover-atomic-json-')) + t.after(() => fs.rm(directory, { recursive: true, force: true })) + const filePath = path.join(directory, 'state.json') + await fs.writeFile(filePath, '{"current":true}\n') + + await assert.rejects(replaceJsonFile(filePath, { unsupported: 1n }), TypeError) + + assert.equal(await fs.readFile(filePath, 'utf8'), '{"current":true}\n') + assert.deepEqual(await fs.readdir(directory), ['state.json']) +}) + +test('cleans the private temporary file when rename fails', async (t) => { + const directory = await fs.mkdtemp(path.join(os.tmpdir(), 'markover-atomic-json-')) + t.after(() => fs.rm(directory, { recursive: true, force: true })) + const destination = path.join(directory, 'state.json') + await fs.mkdir(destination) + + await assert.rejects(replaceJsonFile(destination, { current: true })) + + assert.deepEqual(await fs.readdir(directory), ['state.json']) + assert.deepEqual(await fs.readdir(destination), []) +}) diff --git a/test/review-store.test.ts b/test/review-store.test.ts index 51e97ab4..4791bce2 100644 --- a/test/review-store.test.ts +++ b/test/review-store.test.ts @@ -2025,6 +2025,12 @@ test('rejects unsafe IDs and leaves no temporary files', async (t) => { const entries = await fs.readdir(store.reviewDirectory(created.review.id)) assert.deepEqual(entries, ['review.json']) + if (process.platform !== 'win32') { + assert.equal( + (await fs.stat(store.reviewPath(created.review.id))).mode & 0o777, + 0o600 + ) + } }) test('publishes complete sessions and ignores incomplete review directories', async (t) => { diff --git a/test/settings.test.ts b/test/settings.test.ts index 30620d4d..37064a43 100644 --- a/test/settings.test.ts +++ b/test/settings.test.ts @@ -418,6 +418,9 @@ test('settings store serializes rapid updates without losing the latest values', assert.equal(saved.appearance, 'dark') assert.equal(saved.treeDensity, 'compact') assert.deepEqual(await fs.readdir(directory), ['settings.json']) + if (process.platform !== 'win32') { + assert.equal((await fs.stat(filePath)).mode & 0o777, 0o600) + } }) test('settings store loads offline manual edits after restart', async (t) => {