From dba7ce8a59097095837a64f05b7b421c2b806362 Mon Sep 17 00:00:00 2001 From: chinawch007 Date: Thu, 3 Sep 2026 18:42:52 +0800 Subject: [PATCH 1/5] fix(storage): unify atomic writes for JSON config stores settings-store and mcp-config-store replaced their JSON files without synchronizing the temporary file or, where supported, the published directory entry. The settings, MCP config, and credentials stores also carried different temp naming, permission, cleanup, and synchronization behavior. Consolidate these three legacy JSON config stores on a shared writeAtomicFile helper with exclusive UUID-named temporary files, owner-only modes, handle-bound chmod before file synchronization, atomic rename, parent-directory synchronization where supported, and pre-publication cleanup that preserves the original error. This helper is intentionally scoped to settings.json, mcp.json, and credentials.json. marker-file retains its create-via-link publication contract, while the Runtime Policy document writer retains bounded serialization, typed commit-outcome errors, and stale-temp recovery. The synchronization sequence is the strongest ordering available through the current Node filesystem APIs, not a platform-uniform power-loss guarantee. Linux remains conditional on the filesystem and hardware fsync contract; macOS does not receive F_FULLFSYNC through Node; Windows synchronizes file contents but has no equivalent parent-directory fence. Add fault-injection, permission, cleanup, symlink, and ordering coverage, and update the generated Windows test and workflow inventories. Refs #4285 Generated-by: OpenAI Codex --- .github/workflows/windows-recovery.yml | 2 +- docs/windows-test-inventory.md | 1 + .../src/__tests__/atomic-file-write.test.ts | 266 ++++++++++++++++++ .../src/__tests__/credential-store.test.ts | 6 +- .../src/__tests__/mcp-config-store.test.ts | 11 +- .../settings-store-onboarding.test.ts | 22 +- packages/storage/src/atomic-file-write.ts | 130 +++++++++ packages/storage/src/credential-store.ts | 74 +---- packages/storage/src/mcp-config-store.ts | 27 +- packages/storage/src/settings-store.ts | 12 +- 10 files changed, 458 insertions(+), 93 deletions(-) create mode 100644 packages/storage/src/__tests__/atomic-file-write.test.ts create mode 100644 packages/storage/src/atomic-file-write.ts diff --git a/.github/workflows/windows-recovery.yml b/.github/workflows/windows-recovery.yml index aa7598430b..886404f416 100644 --- a/.github/workflows/windows-recovery.yml +++ b/.github/workflows/windows-recovery.yml @@ -116,7 +116,7 @@ on: - 'packages/storage/src/artifact-store.ts' - 'packages/storage/src/artifact-writer-bootstrap-lock.ts' - 'packages/storage/src/artifact-writer-lock.ts' - - 'packages/storage/src/credential-store.ts' + - 'packages/storage/src/atomic-file-write.ts' - 'packages/storage/src/file-lifetime-owner.ts' - 'packages/storage/src/managed-dependency-environment.ts' - 'packages/storage/src/marker-file.ts' diff --git a/docs/windows-test-inventory.md b/docs/windows-test-inventory.md index 50f0f66928..529364c1f9 100644 --- a/docs/windows-test-inventory.md +++ b/docs/windows-test-inventory.md @@ -98,6 +98,7 @@ Total Windows-excluded declarations: **77** | portable-candidate | `packages/storage/src/__tests__/runtime-policy-stores.test.ts` disabling proxy authentication commits policy before deleting its credential | `process.platform === 'win32' ? 'POSIX file handles are required to inject persistence failures' : false` | | platform-contract | `packages/storage/src/__tests__/runtime-policy-stores.test.ts` successor recovery removes credentials orphaned by an interrupted connection removal | `process.platform === 'win32' ? 'POSIX permissions are required to inject a persistence failure' : false` | | platform-contract | `packages/storage/src/__tests__/runtime-policy-stores.test.ts` fails closed on final symlinks, FIFOs, and oversized documents without changing bytes | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/settings-store-onboarding.test.ts` writes settings.json owner-only (0600) and leaves no temp file behind | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/stable-storage.test.ts` rejects a symlink instead of following it | `process.platform === 'win32' ? 'POSIX no-follow semantics are required' : false` | | platform-contract | `packages/storage/src/__tests__/usage-stores.test.ts` classifies a renamed or replaced live root as a draining persistence failure | `process.platform === 'win32' ? 'Windows does not permit renaming a directory with an open SQLite database' : false` | | platform-contract | `packages/storage/src/__tests__/workspace-identity.test.ts` an unmarked read-only workspace fails without leaving marker state | `process.platform === 'win32' ? 'POSIX permissions are required to create a read-only workspace fixture' : false` | diff --git a/packages/storage/src/__tests__/atomic-file-write.test.ts b/packages/storage/src/__tests__/atomic-file-write.test.ts new file mode 100644 index 0000000000..04fcab5fb9 --- /dev/null +++ b/packages/storage/src/__tests__/atomic-file-write.test.ts @@ -0,0 +1,266 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import assert from 'node:assert/strict'; +import { + chmod, + lstat, + mkdir, + mkdtemp, + open, + readdir, + readFile, + rm, + stat, + symlink, + writeFile, +} from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { describe, test } from 'node:test'; +import { + writeAtomicFile, + type AtomicFileWriteDependencies, + type AtomicFileWriteHandle, +} from '../atomic-file-write.js'; + +const isPosix = process.platform !== 'win32'; + +async function withTempDir(fn: (dir: string) => Promise): Promise { + const dir = await mkdtemp(join(tmpdir(), 'maka-atomic-write-')); + try { + return await fn(dir); + } finally { + await rm(dir, { recursive: true, force: true }); + } +} + +describe('writeAtomicFile', () => { + test('writes the exact bytes and leaves no temp file behind', async () => { + await withTempDir(async (dir) => { + const path = join(dir, 'settings.json'); + await writeAtomicFile(path, '{"a":1}\n'); + assert.equal(await readFile(path, 'utf8'), '{"a":1}\n'); + assert.deepEqual(await readdir(dir), ['settings.json']); + }); + }); + + for (const failurePhase of ['write', 'sync', 'close'] as const) { + test(`removes its temp file and rethrows after a ${failurePhase} failure`, async () => { + await withTempDir(async (dir) => { + const path = join(dir, 'settings.json'); + const temporaryPath = `${path}.fault.tmp`; + const fault = new Error(`${failurePhase} failed`); + await assert.rejects( + () => + writeAtomicFile(path, '{"a":1}\n', undefined, { + randomUUID: () => 'fault', + open: faultingOpen(temporaryPath, failurePhase, fault), + }), + fault, + ); + assert.deepEqual(await readdir(dir), []); + }); + }); + } + + test('removes its temp file and rethrows after a chmod failure', { skip: !isPosix }, async () => { + await withTempDir(async (dir) => { + const path = join(dir, 'settings.json'); + const temporaryPath = `${path}.fault.tmp`; + const fault = new Error('chmod failed'); + await assert.rejects( + () => + writeAtomicFile(path, '{"a":1}\n', undefined, { + randomUUID: () => 'fault', + open: faultingOpen(temporaryPath, 'chmod', fault), + }), + fault, + ); + assert.deepEqual(await readdir(dir), []); + }); + }); + + test('sets the final mode before synchronizing the temp file', async () => { + await withTempDir(async (dir) => { + const path = join(dir, 'settings.json'); + const phases: string[] = []; + await writeAtomicFile(path, '{"a":1}\n', undefined, { + open: async (temporaryPath, flags, mode) => { + const handle = await open(temporaryPath, flags, mode); + return { + writeFile: async (data, encoding) => { + phases.push('write'); + await handle.writeFile(data, encoding); + }, + chmod: async (nextMode) => { + phases.push('chmod'); + await handle.chmod(nextMode); + }, + sync: async () => { + phases.push('sync'); + await handle.sync(); + }, + close: async () => { + phases.push('close'); + await handle.close(); + }, + }; + }, + syncDirectory: async () => { + phases.push('sync-directory'); + }, + }); + assert.deepEqual( + phases, + isPosix + ? ['write', 'chmod', 'sync', 'close', 'sync-directory'] + : ['write', 'sync', 'close', 'sync-directory'], + ); + }); + }); + + test('propagates a directory-fsync failure after the rename with the new file already in place', async () => { + await withTempDir(async (dir) => { + const path = join(dir, 'settings.json'); + const fault = new Error('dirsync failed'); + await assert.rejects( + () => + writeAtomicFile(path, '{"a":1}\n', undefined, { + syncDirectory: async () => { + throw fault; + }, + }), + fault, + ); + // rename is the commit point: the replacement is live (readers get the + // new bytes) even though the post-rename durability fence failed loud. + assert.equal(await readFile(path, 'utf8'), '{"a":1}\n'); + assert.deepEqual(await readdir(dir), ['settings.json']); + }); + }); + + test('creates the target 0600 on POSIX', { skip: !isPosix }, async () => { + await withTempDir(async (dir) => { + const path = join(dir, 'settings.json'); + await writeAtomicFile(path, '{}\n'); + assert.equal((await stat(path)).mode & 0o777, 0o600); + }); + }); + + test('re-chmods a pre-existing world-readable target to 0600 on the next write', { + skip: !isPosix, + }, async () => { + await withTempDir(async (dir) => { + const path = join(dir, 'settings.json'); + // A file created with a loose mode by an older writer. + await writeFile(path, '{}\n', { encoding: 'utf8', mode: 0o644 }); + await writeAtomicFile(path, '{"a":1}\n'); + assert.equal((await stat(path)).mode & 0o777, 0o600); + assert.equal(await readFile(path, 'utf8'), '{"a":1}\n'); + }); + }); + + test("dir 'harden' creates a 0700 directory chain for a nested target", { + skip: !isPosix, + }, async () => { + await withTempDir(async (dir) => { + const path = join(dir, 'secrets', 'sub', 'credentials.json'); + await writeAtomicFile(path, '{}\n', { fileMode: 0o600, dir: 'harden' }); + assert.equal((await stat(join(dir, 'secrets'))).mode & 0o777, 0o700); + assert.equal((await stat(join(dir, 'secrets', 'sub'))).mode & 0o777, 0o700); + assert.equal((await stat(path)).mode & 0o777, 0o600); + }); + }); + + test("dir 'harden' re-chmods a pre-existing world-accessible directory to 0700", { + skip: !isPosix, + }, async () => { + await withTempDir(async (dir) => { + const loose = join(dir, 'loose'); + await mkdir(loose, { recursive: true, mode: 0o777 }); + await chmod(loose, 0o777); // mkdir's mode only applies on creation + await writeAtomicFile(join(loose, 'credentials.json'), '{}\n', { dir: 'harden' }); + // hardenDirectory re-chmods an existing dir; a failure would fail the + // write rather than leave secrets in a world-readable directory. + assert.equal((await stat(loose)).mode & 0o777, 0o700); + }); + }); + + test('refuses to write through a pre-planted symlink at the temp path', { + skip: !isPosix, + }, async () => { + await withTempDir(async (dir) => { + const path = join(dir, 'credentials.json'); + const plantedTarget = join(dir, 'planted-target.json'); + await writeFile(plantedTarget, 'do not touch\n', 'utf8'); + // The injected randomUUID makes the unpredictable temp path knowable, + // which is exactly the attacker model 'wx'/O_EXCL answers. + await symlink(plantedTarget, `${path}.planted.tmp`); + await assert.rejects( + () => writeAtomicFile(path, '{}\n', undefined, { randomUUID: () => 'planted' }), + { code: 'EEXIST' }, + ); + assert.equal(await readFile(plantedTarget, 'utf8'), 'do not touch\n'); + assert.equal(await stat(path).catch(() => null), null); + // Cleanup removes only what the writer created: the planted entry is + // still a symlink, exactly where it was. + assert.equal((await lstat(`${path}.planted.tmp`)).isSymbolicLink(), true); + }); + }); +}); + +function faultingOpen( + temporaryPath: string, + failurePhase: 'write' | 'chmod' | 'sync' | 'close', + fault: Error, +): AtomicFileWriteDependencies['open'] { + return async (path, flags, mode) => { + const handle = await open(path, flags, mode); + if (path !== temporaryPath) return handle; + + let closeFailed = false; + const wrapped: AtomicFileWriteHandle = { + writeFile: async (data, encoding) => { + if (failurePhase === 'write') { + await handle.writeFile(data.slice(0, 1), encoding); + throw fault; + } + await handle.writeFile(data, encoding); + }, + chmod: async (mode) => { + if (failurePhase === 'chmod') throw fault; + await handle.chmod(mode); + }, + sync: async () => { + if (failurePhase === 'sync') throw fault; + await handle.sync(); + }, + close: async () => { + if (failurePhase === 'close' && !closeFailed) { + closeFailed = true; + await handle.close(); + throw fault; + } + await handle.close(); + }, + }; + return wrapped; + }; +} diff --git a/packages/storage/src/__tests__/credential-store.test.ts b/packages/storage/src/__tests__/credential-store.test.ts index 9f6e195a3b..4f577f8b98 100644 --- a/packages/storage/src/__tests__/credential-store.test.ts +++ b/packages/storage/src/__tests__/credential-store.test.ts @@ -144,9 +144,9 @@ describe('FileCredentialStore', () => { await chmod(dir, 0o777); // a loose dir that predates the hardening const store = createFileCredentialStore(dir); await store.setSecret('a', 'api_key', 'k'); - // ensureSecretDir re-chmods an existing dir (mkdir's mode only applies on - // creation); the writer and the lock share it, so the lock can't leave - // the dir loose either. + // hardenDirectory re-chmods an existing dir (mkdir's mode only applies + // on creation); the writer and the lock share it, so the lock can't + // leave the dir loose either. assert.equal((await stat(dir)).mode & 0o777, 0o700); }); }); diff --git a/packages/storage/src/__tests__/mcp-config-store.test.ts b/packages/storage/src/__tests__/mcp-config-store.test.ts index faff8d4dab..cf9fa323a1 100644 --- a/packages/storage/src/__tests__/mcp-config-store.test.ts +++ b/packages/storage/src/__tests__/mcp-config-store.test.ts @@ -19,7 +19,7 @@ import assert from 'node:assert/strict'; import { fork } from 'node:child_process'; -import { mkdtemp, readFile, rm, stat, writeFile } from 'node:fs/promises'; +import { mkdtemp, readdir, readFile, rm, stat, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, test } from 'node:test'; @@ -58,6 +58,15 @@ test('creates and atomically updates a Claude-compatible mcp.json', async () => assert.deepEqual((await store.get()).mcpServers, {}); }); +test('leaves no temp file behind after writes', async () => { + const root = await tempRoot(); + const store = createMcpConfigStore(root); + await store.upsert('filesystem', { command: 'npx', args: ['-y', 'server'] }); + await store.remove('filesystem'); + const strays = (await readdir(root)).filter((entry) => entry.endsWith('.tmp')); + assert.deepEqual(strays, []); +}); + test('reads version 1 without rewriting and persists version 3 on the next mutation', async () => { const root = await tempRoot(); const path = join(root, 'mcp.json'); diff --git a/packages/storage/src/__tests__/settings-store-onboarding.test.ts b/packages/storage/src/__tests__/settings-store-onboarding.test.ts index 525b41af13..7dfac85efe 100644 --- a/packages/storage/src/__tests__/settings-store-onboarding.test.ts +++ b/packages/storage/src/__tests__/settings-store-onboarding.test.ts @@ -30,7 +30,7 @@ import { strict as assert } from 'node:assert'; import { describe, it } from 'node:test'; -import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { mkdtemp, readdir, readFile, rm, stat, writeFile } from 'node:fs/promises'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { createSettingsStore } from '../settings-store.js'; @@ -359,4 +359,24 @@ describe('SettingsStore.get file recovery', () => { await rm(workspaceRoot, { recursive: true, force: true }); } }); + + it('writes settings.json owner-only (0600) and leaves no temp file behind', { + skip: process.platform === 'win32', + }, async () => { + const workspaceRoot = await mkdtemp(join(tmpdir(), 'maka-settings-mode-')); + try { + const store = createSettingsStore(workspaceRoot); + + await store.get(); // first run writes the defaults + + assert.deepEqual(await readdir(workspaceRoot), ['settings.json']); + assert.equal( + (await stat(join(workspaceRoot, 'settings.json'))).mode & 0o777, + 0o600, + 'settings.json carries plaintext credentials (bot secrets, proxy password)', + ); + } finally { + await rm(workspaceRoot, { recursive: true, force: true }); + } + }); }); diff --git a/packages/storage/src/atomic-file-write.ts b/packages/storage/src/atomic-file-write.ts new file mode 100644 index 0000000000..42645caa49 --- /dev/null +++ b/packages/storage/src/atomic-file-write.ts @@ -0,0 +1,130 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { randomUUID } from 'node:crypto'; +import { chmod, mkdir, open, rename, rm } from 'node:fs/promises'; +import { dirname } from 'node:path'; +import { syncDirectory } from './stable-storage.js'; + +/** + * Owner-only atomic write for JSON config files: an exclusive temp file + * ('wx'/O_EXCL so a pre-planted symlink at the predictable-ish temp path is + * never followed), a durability fence before AND after the atomic rename, and + * temp cleanup on failure. This is the single write-side authority for the + * package's JSON stores (settings / mcp / credentials) so their strictness + * cannot drift apart again. + * + * chmod policy is a documented invariant of this module, not a per-call + * choice: + * - file chmod: fail-loud on POSIX, skipped on Windows (no POSIX mode). + * Applies after the exclusive open so umask can never loosen the file. + * - `dir: 'harden'`: mkdir + fail-closed chmod — we must not write plaintext + * secrets into a directory we could not lock down. Windows is best-effort + * for the same reason as above. + */ + +export interface AtomicFileWriteOptions { + /** Mode for the final file. The temp is opened with it and re-chmod'd, so a + * pre-existing looser target is always tightened on the next write. */ + fileMode?: number; + /** Directory handling. 'none' (default) leaves the directory entirely to + * the caller; 'harden' creates/locks down an owner-only directory before + * the temp is written (the credentials-store semantics). */ + dir?: 'none' | 'harden'; + /** Mode for `dir: 'harden'`. */ + dirMode?: number; +} + +/** The fs surface `writeAtomicFile` needs; injectable for fault-injection + * tests (same pattern as marker-file.ts). */ +export interface AtomicFileWriteHandle { + writeFile(data: string, encoding: 'utf8'): Promise; + chmod(mode: number): Promise; + sync(): Promise; + close(): Promise; +} + +export interface AtomicFileWriteDependencies { + open(path: string, flags: string, mode?: number): Promise; + randomUUID(): string; + syncDirectory(path: string): Promise; +} + +const defaultDependencies: AtomicFileWriteDependencies = { + open, + randomUUID, + syncDirectory, +}; + +export async function writeAtomicFile( + path: string, + contents: string, + options: AtomicFileWriteOptions = {}, + dependencies: Partial = {}, +): Promise { + const deps = { ...defaultDependencies, ...dependencies }; + const fileMode = options.fileMode ?? 0o600; + if (options.dir === 'harden') { + await hardenDirectory(dirname(path), options.dirMode ?? 0o700); + } + const tempPath = `${path}.${deps.randomUUID()}.tmp`; + // Only the entry this call created — and hasn't renamed away — may be + // cleaned up. A pre-existing file or planted symlink the 'wx' open refused + // must not be deleted as "our" temp. + let tempCreated = false; + try { + const handle = await deps.open(tempPath, 'wx', fileMode); + tempCreated = true; + try { + await handle.writeFile(contents, 'utf8'); + if (process.platform !== 'win32') await handle.chmod(fileMode); + await handle.sync(); + await handle.close(); + } catch (error) { + // Release the descriptor best-effort; never let a close failure mask + // the error that actually aborted the write. + await handle.close().catch(() => {}); + throw error; + } + await rename(tempPath, path); + tempCreated = false; + await deps.syncDirectory(dirname(path)); + } catch (error) { + if (tempCreated) { + // Cleanup must never mask the original failure. + await rm(tempPath, { force: true }).catch(() => {}); + } + throw error; + } +} + +/** + * Create (or harden) an owner-only directory: recursive mkdir plus a + * fail-closed chmod, since mkdir's mode only applies on creation. Shared by + * the 'harden' write path and the credentials lock so their directory + * strictness cannot drift apart. + */ +export async function hardenDirectory(dir: string, mode: number = 0o700): Promise { + await mkdir(dir, { recursive: true, mode }); + if (process.platform === 'win32') { + await chmod(dir, mode).catch(() => {}); + return; + } + await chmod(dir, mode); +} diff --git a/packages/storage/src/credential-store.ts b/packages/storage/src/credential-store.ts index 325227e8d5..cecc448347 100644 --- a/packages/storage/src/credential-store.ts +++ b/packages/storage/src/credential-store.ts @@ -17,9 +17,9 @@ * under the License. */ -import { randomUUID } from 'node:crypto'; -import { chmod, mkdir, open, readFile, rename, rm } from 'node:fs/promises'; +import { readFile } from 'node:fs/promises'; import { dirname, join } from 'node:path'; +import { hardenDirectory, writeAtomicFile } from './atomic-file-write.js'; import { withFileUpdateLock } from './file-update-lock.js'; /** @@ -242,67 +242,15 @@ class FileCredentialStore implements CredentialStore { } /** - * Create (or harden) the directory that holds a secret file: 0700, and - * re-chmod a pre-existing looser dir so neither the secret nor the lock can sit - * world-readable. mkdir's mode only applies on creation, so the chmod is what - * fixes an existing dir. On POSIX a chmod failure fails closed (we must not - * write plaintext credentials into a dir we couldn't lock down); no-op on - * Windows. Shared by the writer and the lock so their dir hardening can't drift. - */ -async function ensureSecretDir(dir: string): Promise { - await mkdir(dir, { recursive: true, mode: 0o700 }); - await chmodStrict(dir, 0o700); -} - -/** - * Owner-only atomic write for a credentials file: a 0700 dir, an exclusive - * 0600 temp ('wx'/O_EXCL so we never follow a pre-planted symlink at a - * predictable path), a durability fence before and after the atomic rename, - * and temp cleanup on failure. + * Owner-only atomic write for a credentials file: a hardened 0700 directory, + * an exclusive 0600 temp ('wx'/O_EXCL so we never follow a pre-planted symlink + * at a predictable path), a durability fence before and after the atomic + * rename, and temp cleanup on failure. Implemented by the package-wide + * `writeAtomicFile` authority so its guarantees cannot drift from the other + * JSON stores. */ async function writeSecretFileAtomic(path: string, contents: string): Promise { - await ensureSecretDir(dirname(path)); - const tempPath = `${path}.${randomUUID()}.tmp`; - try { - const handle = await open(tempPath, 'wx', 0o600); - try { - await handle.writeFile(contents, 'utf8'); - await handle.sync(); - } finally { - await handle.close(); - } - await chmodStrict(tempPath, 0o600); - await rename(tempPath, path); - await syncDirectory(dirname(path)); - } catch (error) { - await rm(tempPath, { force: true }); - throw error; - } -} - -async function syncDirectory(path: string): Promise { - if (process.platform === 'win32') return; - const handle = await open(path, 'r'); - try { - await handle.sync(); - } finally { - await handle.close(); - } -} - -/** - * chmod that fails loud on POSIX and is best-effort on Windows. A secret file - * or its directory left looser than intended breaks the 0600/0700 boundary, so - * on POSIX we surface the failure rather than write plaintext into it; Windows - * has no POSIX mode, so a failure there is a no-op. One policy for both the - * secret file (0600) and its directory (0700) so they can't drift apart. - */ -async function chmodStrict(path: string, mode: number): Promise { - if (process.platform === 'win32') { - await chmod(path, mode).catch(() => {}); - return; - } - await chmod(path, mode); + await writeAtomicFile(path, contents, { fileMode: 0o600, dir: 'harden' }); } const LOCK_TIMEOUT_MS = 10_000; @@ -338,7 +286,9 @@ export async function withCredentialFileLock( fn: () => Promise, timeoutMs: number = LOCK_TIMEOUT_MS, ): Promise { - await ensureSecretDir(dirname(targetPath)); + // Same owner-only hardening the writer applies, so the lock directory can + // never sit looser than the secret it guards. + await hardenDirectory(dirname(targetPath)); return withFileUpdateLock(targetPath, fn, timeoutMs); } diff --git a/packages/storage/src/mcp-config-store.ts b/packages/storage/src/mcp-config-store.ts index c362d86eda..b8a1b84391 100644 --- a/packages/storage/src/mcp-config-store.ts +++ b/packages/storage/src/mcp-config-store.ts @@ -17,8 +17,7 @@ * under the License. */ -import { randomUUID } from 'node:crypto'; -import { chmod, mkdir, readFile, rename, rm, writeFile } from 'node:fs/promises'; +import { readFile } from 'node:fs/promises'; import { dirname, join } from 'node:path'; import { MCP_CONFIG_VERSION, @@ -32,6 +31,7 @@ import { type McpServerConfig, type McpStdioServerConfig, } from '@maka/core/mcp'; +import { hardenDirectory, writeAtomicFile } from './atomic-file-write.js'; import { withProcessLifetimeFileUpdateLock } from './process-lifetime-file-update-lock.js'; const MAX_SERVERS = 100; @@ -195,30 +195,15 @@ class FileMcpConfigStore implements McpConfigStore { } private async write(config: McpConfigFile): Promise { - const dir = dirname(this.path); await this.ensureDirectory(); - const tempPath = join(dir, `.mcp-${randomUUID()}.tmp`); - try { - await writeFile(tempPath, `${JSON.stringify(config, null, 2)}\n`, { - encoding: 'utf8', - mode: 0o600, - flag: 'wx', - }); - if (process.platform !== 'win32') await chmod(tempPath, 0o600); - await rename(tempPath, this.path); - if (process.platform !== 'win32') await chmod(this.path, 0o600); - } finally { - await rm(tempPath, { force: true }).catch(() => {}); - } + await writeAtomicFile(this.path, `${JSON.stringify(config, null, 2)}\n`, { + fileMode: 0o600, + }); } private ensureDirectory(): Promise { if (this.directoryReady) return this.directoryReady; - const dir = dirname(this.path); - const ready = (async () => { - await mkdir(dir, { recursive: true, mode: 0o700 }); - if (process.platform !== 'win32') await chmod(dir, 0o700); - })(); + const ready = hardenDirectory(dirname(this.path), 0o700); this.directoryReady = ready; void ready.catch(() => { if (this.directoryReady === ready) this.directoryReady = undefined; diff --git a/packages/storage/src/settings-store.ts b/packages/storage/src/settings-store.ts index 8b892e30df..6e4df1029a 100644 --- a/packages/storage/src/settings-store.ts +++ b/packages/storage/src/settings-store.ts @@ -17,12 +17,13 @@ * under the License. */ -import { mkdir, readFile, rename, writeFile } from 'node:fs/promises'; +import { mkdir, readFile } from 'node:fs/promises'; import { dirname, join } from 'node:path'; import type { AppSettings, UpdateAppSettingsInput } from '@maka/core/settings'; import type { OnboardingMilestone, OnboardingMilestoneId } from '@maka/core/onboarding'; import { createDefaultSettings, mergeSettings, normalizeSettings } from '@maka/core/settings'; import { sanitizeOnboardingMilestones } from '@maka/core/onboarding'; +import { writeAtomicFile } from './atomic-file-write.js'; /** * A conditional write's patch, either fixed or derived from the state the @@ -189,10 +190,13 @@ class FileSettingsStore implements SettingsStore { } private async write(settings: AppSettings): Promise { + // workspaceRoot is a user-owned directory; keep the plain mkdir and do + // not impose 0700 on it. The file itself is 0600 because settings.json + // carries plaintext credentials (bot secrets, proxy password). await mkdir(dirname(this.settingsPath), { recursive: true }); - const tempPath = `${this.settingsPath}.${process.pid}.${Date.now()}.tmp`; - await writeFile(tempPath, JSON.stringify(settings, null, 2) + '\n', 'utf8'); - await rename(tempPath, this.settingsPath); + await writeAtomicFile(this.settingsPath, JSON.stringify(settings, null, 2) + '\n', { + fileMode: 0o600, + }); } private withQueue(operation: () => Promise): Promise { From 3d7b607ae23faaad120aa165e4e933743335d35d Mon Sep 17 00:00:00 2001 From: chinawch007 Date: Fri, 4 Sep 2026 21:10:41 +0800 Subject: [PATCH 2/5] test(windows): inventory atomic writer skips Use the literal Windows exclusion recognized by the generated inventory for the six POSIX-only atomic writer tests. This keeps the skip policy visible to CI and updates the checked-in inventory. Refs #4285 Generated-by: OpenAI Codex --- docs/windows-test-inventory.md | 10 ++++++++-- .../src/__tests__/atomic-file-write.test.ts | 14 ++++++++------ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/docs/windows-test-inventory.md b/docs/windows-test-inventory.md index 529364c1f9..3e8ab0c15c 100644 --- a/docs/windows-test-inventory.md +++ b/docs/windows-test-inventory.md @@ -16,10 +16,10 @@ Locations intentionally omit line numbers so unrelated edits do not invalidate t | Classification | Count | |---|---:| | windows-backend-gap | 27 | -| portable-candidate | 19 | +| portable-candidate | 25 | | platform-contract | 31 | -Total Windows-excluded declarations: **77** +Total Windows-excluded declarations: **83** ## Inventory @@ -77,6 +77,12 @@ Total Windows-excluded declarations: **77** | platform-contract | `packages/runtime/src/__tests__/shell-run-manager.test.ts` settles after root exit when a detached descendant retains inherited stdout | `process.platform === 'win32' ? 'POSIX detached process-group semantics required' : false` | | platform-contract | `packages/runtime/src/__tests__/shell-run-manager.test.ts` keeps the first committed lifecycle cause across Stop and timeout races | `process.platform === 'win32' ? 'Windows tree termination has no graceful SIGTERM phase' : false` | | platform-contract | `packages/runtime/src/__tests__/shell-run-manager.test.ts` keeps SIGTERM final output and escalates an ignored SIGTERM without leaking slots | `process.platform === 'win32' ? 'Windows tree termination has no graceful SIGTERM phase' : false` | +| portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` removes its temp file and rethrows after a chmod failure | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` creates the target 0600 on POSIX | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` re-chmods a pre-existing world-readable target to 0600 on the next write | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` dir 'harden' creates a 0700 directory chain for a nested target | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` dir 'harden' re-chmods a pre-existing world-accessible directory to 0700 | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` refuses to write through a pre-planted symlink at the temp path | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/managed-dependency-environment.test.ts` accepts a POSIX package bin symlink whose target remains inside the dependency root | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/managed-dependency-environment.test.ts` isolates published POSIX content from a producer-retained writable handle | `process.platform === 'win32'` | | platform-contract | `packages/storage/src/__tests__/operational-state-store.test.ts` does not classify a SQLite write failure as a migration blocker | `process.platform === 'win32' ? 'POSIX permissions are required to make the SQLite database read-only' : false` | diff --git a/packages/storage/src/__tests__/atomic-file-write.test.ts b/packages/storage/src/__tests__/atomic-file-write.test.ts index 04fcab5fb9..b4bb92fec1 100644 --- a/packages/storage/src/__tests__/atomic-file-write.test.ts +++ b/packages/storage/src/__tests__/atomic-file-write.test.ts @@ -80,7 +80,9 @@ describe('writeAtomicFile', () => { }); } - test('removes its temp file and rethrows after a chmod failure', { skip: !isPosix }, async () => { + test('removes its temp file and rethrows after a chmod failure', { + skip: process.platform === 'win32', + }, async () => { await withTempDir(async (dir) => { const path = join(dir, 'settings.json'); const temporaryPath = `${path}.fault.tmp`; @@ -156,7 +158,7 @@ describe('writeAtomicFile', () => { }); }); - test('creates the target 0600 on POSIX', { skip: !isPosix }, async () => { + test('creates the target 0600 on POSIX', { skip: process.platform === 'win32' }, async () => { await withTempDir(async (dir) => { const path = join(dir, 'settings.json'); await writeAtomicFile(path, '{}\n'); @@ -165,7 +167,7 @@ describe('writeAtomicFile', () => { }); test('re-chmods a pre-existing world-readable target to 0600 on the next write', { - skip: !isPosix, + skip: process.platform === 'win32', }, async () => { await withTempDir(async (dir) => { const path = join(dir, 'settings.json'); @@ -178,7 +180,7 @@ describe('writeAtomicFile', () => { }); test("dir 'harden' creates a 0700 directory chain for a nested target", { - skip: !isPosix, + skip: process.platform === 'win32', }, async () => { await withTempDir(async (dir) => { const path = join(dir, 'secrets', 'sub', 'credentials.json'); @@ -190,7 +192,7 @@ describe('writeAtomicFile', () => { }); test("dir 'harden' re-chmods a pre-existing world-accessible directory to 0700", { - skip: !isPosix, + skip: process.platform === 'win32', }, async () => { await withTempDir(async (dir) => { const loose = join(dir, 'loose'); @@ -204,7 +206,7 @@ describe('writeAtomicFile', () => { }); test('refuses to write through a pre-planted symlink at the temp path', { - skip: !isPosix, + skip: process.platform === 'win32', }, async () => { await withTempDir(async (dir) => { const path = join(dir, 'credentials.json'); From 5f7b1a6b59bcc62161c7ddf5ebb4f31c54c36622 Mon Sep 17 00:00:00 2001 From: chinawch007 Date: Fri, 4 Sep 2026 21:11:56 +0800 Subject: [PATCH 3/5] fix(storage): distinguish post-rename sync failures Treat rename as the publication point for atomic JSON writes. If the following directory sync fails, report a typed commit-outcome-unknown error with the original failure as its cause so callers know to reload before retrying. Refs #4285 Generated-by: OpenAI Codex --- .../storage/src/__tests__/atomic-file-write.test.ts | 13 ++++++++++--- packages/storage/src/atomic-file-write.ts | 12 ++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/packages/storage/src/__tests__/atomic-file-write.test.ts b/packages/storage/src/__tests__/atomic-file-write.test.ts index b4bb92fec1..b22472ff6f 100644 --- a/packages/storage/src/__tests__/atomic-file-write.test.ts +++ b/packages/storage/src/__tests__/atomic-file-write.test.ts @@ -35,6 +35,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { describe, test } from 'node:test'; import { + AtomicFileWriteCommitUnknownError, writeAtomicFile, type AtomicFileWriteDependencies, type AtomicFileWriteHandle, @@ -138,7 +139,7 @@ describe('writeAtomicFile', () => { }); }); - test('propagates a directory-fsync failure after the rename with the new file already in place', async () => { + test('reports an unknown commit outcome when directory fsync fails after publication', async () => { await withTempDir(async (dir) => { const path = join(dir, 'settings.json'); const fault = new Error('dirsync failed'); @@ -149,10 +150,16 @@ describe('writeAtomicFile', () => { throw fault; }, }), - fault, + (error: unknown) => { + assert.ok(error instanceof AtomicFileWriteCommitUnknownError); + assert.equal(error.published, true); + assert.equal(error.cause, fault); + assert.match(error.message, /reload before retrying/); + return true; + }, ); // rename is the commit point: the replacement is live (readers get the - // new bytes) even though the post-rename durability fence failed loud. + // new bytes) even though its durability is not known to the caller. assert.equal(await readFile(path, 'utf8'), '{"a":1}\n'); assert.deepEqual(await readdir(dir), ['settings.json']); }); diff --git a/packages/storage/src/atomic-file-write.ts b/packages/storage/src/atomic-file-write.ts index 42645caa49..9a689fe4c6 100644 --- a/packages/storage/src/atomic-file-write.ts +++ b/packages/storage/src/atomic-file-write.ts @@ -72,6 +72,15 @@ const defaultDependencies: AtomicFileWriteDependencies = { syncDirectory, }; +export class AtomicFileWriteCommitUnknownError extends Error { + readonly published = true; + + constructor(options: { cause: unknown }) { + super('Atomic file commit outcome is unknown; reload before retrying', options); + this.name = 'AtomicFileWriteCommitUnknownError'; + } +} + export async function writeAtomicFile( path: string, contents: string, @@ -88,6 +97,7 @@ export async function writeAtomicFile( // cleaned up. A pre-existing file or planted symlink the 'wx' open refused // must not be deleted as "our" temp. let tempCreated = false; + let published = false; try { const handle = await deps.open(tempPath, 'wx', fileMode); tempCreated = true; @@ -103,6 +113,7 @@ export async function writeAtomicFile( throw error; } await rename(tempPath, path); + published = true; tempCreated = false; await deps.syncDirectory(dirname(path)); } catch (error) { @@ -110,6 +121,7 @@ export async function writeAtomicFile( // Cleanup must never mask the original failure. await rm(tempPath, { force: true }).catch(() => {}); } + if (published) throw new AtomicFileWriteCommitUnknownError({ cause: error }); throw error; } } From 688ef2f01df45af7c02ebff137d1af9cab576e8d Mon Sep 17 00:00:00 2001 From: chinawch007 Date: Fri, 4 Sep 2026 21:14:52 +0800 Subject: [PATCH 4/5] refactor(storage): clarify atomic writer boundaries Keep directory creation and hardening with each store instead of exposing single-caller policy through the shared writer. Preserve hidden temporary entries, document the platform-specific synchronization guarantees, and align the settings directory comment with the MCP store's independent policy. Refs #4285 Generated-by: OpenAI Codex --- docs/windows-test-inventory.md | 4 +- .../src/__tests__/atomic-file-write.test.ts | 25 ++++++----- packages/storage/src/atomic-file-write.ts | 42 ++++++++----------- packages/storage/src/credential-store.ts | 13 +++--- packages/storage/src/settings-store.ts | 7 ++-- 5 files changed, 42 insertions(+), 49 deletions(-) diff --git a/docs/windows-test-inventory.md b/docs/windows-test-inventory.md index 3e8ab0c15c..6538ce2fa9 100644 --- a/docs/windows-test-inventory.md +++ b/docs/windows-test-inventory.md @@ -80,8 +80,8 @@ Total Windows-excluded declarations: **83** | portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` removes its temp file and rethrows after a chmod failure | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` creates the target 0600 on POSIX | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` re-chmods a pre-existing world-readable target to 0600 on the next write | `process.platform === 'win32'` | -| portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` dir 'harden' creates a 0700 directory chain for a nested target | `process.platform === 'win32'` | -| portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` dir 'harden' re-chmods a pre-existing world-accessible directory to 0700 | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` hardenDirectory creates a 0700 directory chain | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` hardenDirectory re-chmods a pre-existing world-accessible directory to 0700 | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` refuses to write through a pre-planted symlink at the temp path | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/managed-dependency-environment.test.ts` accepts a POSIX package bin symlink whose target remains inside the dependency root | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/managed-dependency-environment.test.ts` isolates published POSIX content from a producer-retained writable handle | `process.platform === 'win32'` | diff --git a/packages/storage/src/__tests__/atomic-file-write.test.ts b/packages/storage/src/__tests__/atomic-file-write.test.ts index b22472ff6f..aa59b509fb 100644 --- a/packages/storage/src/__tests__/atomic-file-write.test.ts +++ b/packages/storage/src/__tests__/atomic-file-write.test.ts @@ -36,6 +36,7 @@ import { join } from 'node:path'; import { describe, test } from 'node:test'; import { AtomicFileWriteCommitUnknownError, + hardenDirectory, writeAtomicFile, type AtomicFileWriteDependencies, type AtomicFileWriteHandle, @@ -66,7 +67,7 @@ describe('writeAtomicFile', () => { test(`removes its temp file and rethrows after a ${failurePhase} failure`, async () => { await withTempDir(async (dir) => { const path = join(dir, 'settings.json'); - const temporaryPath = `${path}.fault.tmp`; + const temporaryPath = join(dir, '.settings.json.fault.tmp'); const fault = new Error(`${failurePhase} failed`); await assert.rejects( () => @@ -86,7 +87,7 @@ describe('writeAtomicFile', () => { }, async () => { await withTempDir(async (dir) => { const path = join(dir, 'settings.json'); - const temporaryPath = `${path}.fault.tmp`; + const temporaryPath = join(dir, '.settings.json.fault.tmp'); const fault = new Error('chmod failed'); await assert.rejects( () => @@ -186,28 +187,25 @@ describe('writeAtomicFile', () => { }); }); - test("dir 'harden' creates a 0700 directory chain for a nested target", { + test('hardenDirectory creates a 0700 directory chain', { skip: process.platform === 'win32', }, async () => { await withTempDir(async (dir) => { - const path = join(dir, 'secrets', 'sub', 'credentials.json'); - await writeAtomicFile(path, '{}\n', { fileMode: 0o600, dir: 'harden' }); + const targetDir = join(dir, 'secrets', 'sub'); + await hardenDirectory(targetDir); assert.equal((await stat(join(dir, 'secrets'))).mode & 0o777, 0o700); - assert.equal((await stat(join(dir, 'secrets', 'sub'))).mode & 0o777, 0o700); - assert.equal((await stat(path)).mode & 0o777, 0o600); + assert.equal((await stat(targetDir)).mode & 0o777, 0o700); }); }); - test("dir 'harden' re-chmods a pre-existing world-accessible directory to 0700", { + test('hardenDirectory re-chmods a pre-existing world-accessible directory to 0700', { skip: process.platform === 'win32', }, async () => { await withTempDir(async (dir) => { const loose = join(dir, 'loose'); await mkdir(loose, { recursive: true, mode: 0o777 }); await chmod(loose, 0o777); // mkdir's mode only applies on creation - await writeAtomicFile(join(loose, 'credentials.json'), '{}\n', { dir: 'harden' }); - // hardenDirectory re-chmods an existing dir; a failure would fail the - // write rather than leave secrets in a world-readable directory. + await hardenDirectory(loose); assert.equal((await stat(loose)).mode & 0o777, 0o700); }); }); @@ -221,7 +219,8 @@ describe('writeAtomicFile', () => { await writeFile(plantedTarget, 'do not touch\n', 'utf8'); // The injected randomUUID makes the unpredictable temp path knowable, // which is exactly the attacker model 'wx'/O_EXCL answers. - await symlink(plantedTarget, `${path}.planted.tmp`); + const plantedTemp = join(dir, '.credentials.json.planted.tmp'); + await symlink(plantedTarget, plantedTemp); await assert.rejects( () => writeAtomicFile(path, '{}\n', undefined, { randomUUID: () => 'planted' }), { code: 'EEXIST' }, @@ -230,7 +229,7 @@ describe('writeAtomicFile', () => { assert.equal(await stat(path).catch(() => null), null); // Cleanup removes only what the writer created: the planted entry is // still a symlink, exactly where it was. - assert.equal((await lstat(`${path}.planted.tmp`)).isSymbolicLink(), true); + assert.equal((await lstat(plantedTemp)).isSymbolicLink(), true); }); }); }); diff --git a/packages/storage/src/atomic-file-write.ts b/packages/storage/src/atomic-file-write.ts index 9a689fe4c6..7cfc44f97e 100644 --- a/packages/storage/src/atomic-file-write.ts +++ b/packages/storage/src/atomic-file-write.ts @@ -19,36 +19,34 @@ import { randomUUID } from 'node:crypto'; import { chmod, mkdir, open, rename, rm } from 'node:fs/promises'; -import { dirname } from 'node:path'; +import { basename, dirname, join } from 'node:path'; import { syncDirectory } from './stable-storage.js'; /** - * Owner-only atomic write for JSON config files: an exclusive temp file - * ('wx'/O_EXCL so a pre-planted symlink at the predictable-ish temp path is - * never followed), a durability fence before AND after the atomic rename, and - * temp cleanup on failure. This is the single write-side authority for the - * package's JSON stores (settings / mcp / credentials) so their strictness - * cannot drift apart again. + * Owner-only atomic writer shared by the legacy settings, MCP, and credentials + * JSON stores. It publishes an exclusive hidden temp file with rename and + * removes that temp on failures before publication. * - * chmod policy is a documented invariant of this module, not a per-call + * Durability support depends on the platform and storage stack: + * - Linux and POSIX systems other than macOS: sync the temp file before + * rename and fsync the parent directory afterwards. Persistence still + * depends on the filesystem, mount, device, and hardware honoring them. + * - macOS: use Node's ordinary fsync operations; Node does not expose + * F_FULLFSYNC here, so this is not a uniform sudden-power-loss guarantee. + * - Windows: sync the temp file before rename, but parent-directory sync is a + * no-op because Node does not provide an equivalent directory fence. + * + * File chmod policy is a documented invariant of this module, not a per-call * choice: * - file chmod: fail-loud on POSIX, skipped on Windows (no POSIX mode). * Applies after the exclusive open so umask can never loosen the file. - * - `dir: 'harden'`: mkdir + fail-closed chmod — we must not write plaintext - * secrets into a directory we could not lock down. Windows is best-effort - * for the same reason as above. + * Directory creation and permission policy belong to each caller. */ export interface AtomicFileWriteOptions { /** Mode for the final file. The temp is opened with it and re-chmod'd, so a * pre-existing looser target is always tightened on the next write. */ fileMode?: number; - /** Directory handling. 'none' (default) leaves the directory entirely to - * the caller; 'harden' creates/locks down an owner-only directory before - * the temp is written (the credentials-store semantics). */ - dir?: 'none' | 'harden'; - /** Mode for `dir: 'harden'`. */ - dirMode?: number; } /** The fs surface `writeAtomicFile` needs; injectable for fault-injection @@ -89,10 +87,7 @@ export async function writeAtomicFile( ): Promise { const deps = { ...defaultDependencies, ...dependencies }; const fileMode = options.fileMode ?? 0o600; - if (options.dir === 'harden') { - await hardenDirectory(dirname(path), options.dirMode ?? 0o700); - } - const tempPath = `${path}.${deps.randomUUID()}.tmp`; + const tempPath = join(dirname(path), `.${basename(path)}.${deps.randomUUID()}.tmp`); // Only the entry this call created — and hasn't renamed away — may be // cleaned up. A pre-existing file or planted symlink the 'wx' open refused // must not be deleted as "our" temp. @@ -128,9 +123,8 @@ export async function writeAtomicFile( /** * Create (or harden) an owner-only directory: recursive mkdir plus a - * fail-closed chmod, since mkdir's mode only applies on creation. Shared by - * the 'harden' write path and the credentials lock so their directory - * strictness cannot drift apart. + * fail-closed chmod, since mkdir's mode only applies on creation. Callers that + * store secrets use this before creating their file or update lock. */ export async function hardenDirectory(dir: string, mode: number = 0o700): Promise { await mkdir(dir, { recursive: true, mode }); diff --git a/packages/storage/src/credential-store.ts b/packages/storage/src/credential-store.ts index cecc448347..8c34503f02 100644 --- a/packages/storage/src/credential-store.ts +++ b/packages/storage/src/credential-store.ts @@ -242,15 +242,14 @@ class FileCredentialStore implements CredentialStore { } /** - * Owner-only atomic write for a credentials file: a hardened 0700 directory, - * an exclusive 0600 temp ('wx'/O_EXCL so we never follow a pre-planted symlink - * at a predictable path), a durability fence before and after the atomic - * rename, and temp cleanup on failure. Implemented by the package-wide - * `writeAtomicFile` authority so its guarantees cannot drift from the other - * JSON stores. + * Owner-only atomic write for a credentials file. Directory hardening is an + * explicit credential-store policy; file publication uses the shared legacy + * JSON writer so its mode, synchronization, and cleanup behavior remains + * aligned with settings and MCP config. */ async function writeSecretFileAtomic(path: string, contents: string): Promise { - await writeAtomicFile(path, contents, { fileMode: 0o600, dir: 'harden' }); + await hardenDirectory(dirname(path)); + await writeAtomicFile(path, contents, { fileMode: 0o600 }); } const LOCK_TIMEOUT_MS = 10_000; diff --git a/packages/storage/src/settings-store.ts b/packages/storage/src/settings-store.ts index 6e4df1029a..b180edbba8 100644 --- a/packages/storage/src/settings-store.ts +++ b/packages/storage/src/settings-store.ts @@ -190,9 +190,10 @@ class FileSettingsStore implements SettingsStore { } private async write(settings: AppSettings): Promise { - // workspaceRoot is a user-owned directory; keep the plain mkdir and do - // not impose 0700 on it. The file itself is 0600 because settings.json - // carries plaintext credentials (bot secrets, proxy password). + // SettingsStore does not own the workspace directory's permission policy: + // sibling stores such as MCP config may independently harden the same root. + // Keep creation here plain; settings.json itself is 0600 because it carries + // plaintext credentials (bot secrets, proxy password). await mkdir(dirname(this.settingsPath), { recursive: true }); await writeAtomicFile(this.settingsPath, JSON.stringify(settings, null, 2) + '\n', { fileMode: 0o600, From d318b4ff6ede38712861967886c7e34b9e77692a Mon Sep 17 00:00:00 2001 From: chinawch007 Date: Mon, 7 Sep 2026 12:49:27 +0800 Subject: [PATCH 5/5] refactor(storage): preserve store-owned file modes Restore SettingsStore's historical 0666-and-umask behavior while keeping MCP and credential files explicitly 0600. Make fileMode required so the shared writer represents a real caller-owned policy and applies the effective mode before synchronization. Move hardenDirectory and its coverage to stable-storage, update the two secret-store imports, and refresh the Windows test inventory. Refs #4285 Generated-by: OpenAI Codex --- docs/windows-test-inventory.md | 10 ++--- .../src/__tests__/atomic-file-write.test.ts | 43 ++++--------------- .../settings-store-onboarding.test.ts | 8 ++-- .../src/__tests__/stable-storage.test.ts | 37 +++++++++++++++- packages/storage/src/atomic-file-write.ts | 40 ++++++----------- packages/storage/src/credential-store.ts | 3 +- packages/storage/src/mcp-config-store.ts | 3 +- packages/storage/src/settings-store.ts | 5 +-- packages/storage/src/stable-storage.ts | 16 ++++++- 9 files changed, 88 insertions(+), 77 deletions(-) diff --git a/docs/windows-test-inventory.md b/docs/windows-test-inventory.md index 6538ce2fa9..d2df5ccbce 100644 --- a/docs/windows-test-inventory.md +++ b/docs/windows-test-inventory.md @@ -16,10 +16,10 @@ Locations intentionally omit line numbers so unrelated edits do not invalidate t | Classification | Count | |---|---:| | windows-backend-gap | 27 | -| portable-candidate | 25 | +| portable-candidate | 26 | | platform-contract | 31 | -Total Windows-excluded declarations: **83** +Total Windows-excluded declarations: **84** ## Inventory @@ -80,8 +80,6 @@ Total Windows-excluded declarations: **83** | portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` removes its temp file and rethrows after a chmod failure | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` creates the target 0600 on POSIX | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` re-chmods a pre-existing world-readable target to 0600 on the next write | `process.platform === 'win32'` | -| portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` hardenDirectory creates a 0700 directory chain | `process.platform === 'win32'` | -| portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` hardenDirectory re-chmods a pre-existing world-accessible directory to 0700 | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/atomic-file-write.test.ts` refuses to write through a pre-planted symlink at the temp path | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/managed-dependency-environment.test.ts` accepts a POSIX package bin symlink whose target remains inside the dependency root | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/managed-dependency-environment.test.ts` isolates published POSIX content from a producer-retained writable handle | `process.platform === 'win32'` | @@ -104,8 +102,10 @@ Total Windows-excluded declarations: **83** | portable-candidate | `packages/storage/src/__tests__/runtime-policy-stores.test.ts` disabling proxy authentication commits policy before deleting its credential | `process.platform === 'win32' ? 'POSIX file handles are required to inject persistence failures' : false` | | platform-contract | `packages/storage/src/__tests__/runtime-policy-stores.test.ts` successor recovery removes credentials orphaned by an interrupted connection removal | `process.platform === 'win32' ? 'POSIX permissions are required to inject a persistence failure' : false` | | platform-contract | `packages/storage/src/__tests__/runtime-policy-stores.test.ts` fails closed on final symlinks, FIFOs, and oversized documents without changing bytes | `process.platform === 'win32'` | -| portable-candidate | `packages/storage/src/__tests__/settings-store-onboarding.test.ts` writes settings.json owner-only (0600) and leaves no temp file behind | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/settings-store-onboarding.test.ts` preserves a restrictive umask-derived settings.json mode and leaves no temp file behind | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/stable-storage.test.ts` rejects a symlink instead of following it | `process.platform === 'win32' ? 'POSIX no-follow semantics are required' : false` | +| portable-candidate | `packages/storage/src/__tests__/stable-storage.test.ts` hardenDirectory creates a 0700 directory chain | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/stable-storage.test.ts` hardenDirectory re-chmods a pre-existing world-accessible directory to 0700 | `process.platform === 'win32'` | | platform-contract | `packages/storage/src/__tests__/usage-stores.test.ts` classifies a renamed or replaced live root as a draining persistence failure | `process.platform === 'win32' ? 'Windows does not permit renaming a directory with an open SQLite database' : false` | | platform-contract | `packages/storage/src/__tests__/workspace-identity.test.ts` an unmarked read-only workspace fails without leaving marker state | `process.platform === 'win32' ? 'POSIX permissions are required to create a read-only workspace fixture' : false` | | portable-candidate | `scripts/release-cli-eval-support.test.mjs` preserves the primary process failure when diagnostics cannot be read | `process.platform === 'win32'` | diff --git a/packages/storage/src/__tests__/atomic-file-write.test.ts b/packages/storage/src/__tests__/atomic-file-write.test.ts index aa59b509fb..9427351753 100644 --- a/packages/storage/src/__tests__/atomic-file-write.test.ts +++ b/packages/storage/src/__tests__/atomic-file-write.test.ts @@ -19,9 +19,7 @@ import assert from 'node:assert/strict'; import { - chmod, lstat, - mkdir, mkdtemp, open, readdir, @@ -36,13 +34,13 @@ import { join } from 'node:path'; import { describe, test } from 'node:test'; import { AtomicFileWriteCommitUnknownError, - hardenDirectory, writeAtomicFile, type AtomicFileWriteDependencies, type AtomicFileWriteHandle, } from '../atomic-file-write.js'; const isPosix = process.platform !== 'win32'; +const ownerOnlyFile = { fileMode: 0o600 } as const; async function withTempDir(fn: (dir: string) => Promise): Promise { const dir = await mkdtemp(join(tmpdir(), 'maka-atomic-write-')); @@ -57,7 +55,7 @@ describe('writeAtomicFile', () => { test('writes the exact bytes and leaves no temp file behind', async () => { await withTempDir(async (dir) => { const path = join(dir, 'settings.json'); - await writeAtomicFile(path, '{"a":1}\n'); + await writeAtomicFile(path, '{"a":1}\n', ownerOnlyFile); assert.equal(await readFile(path, 'utf8'), '{"a":1}\n'); assert.deepEqual(await readdir(dir), ['settings.json']); }); @@ -71,7 +69,7 @@ describe('writeAtomicFile', () => { const fault = new Error(`${failurePhase} failed`); await assert.rejects( () => - writeAtomicFile(path, '{"a":1}\n', undefined, { + writeAtomicFile(path, '{"a":1}\n', ownerOnlyFile, { randomUUID: () => 'fault', open: faultingOpen(temporaryPath, failurePhase, fault), }), @@ -91,7 +89,7 @@ describe('writeAtomicFile', () => { const fault = new Error('chmod failed'); await assert.rejects( () => - writeAtomicFile(path, '{"a":1}\n', undefined, { + writeAtomicFile(path, '{"a":1}\n', ownerOnlyFile, { randomUUID: () => 'fault', open: faultingOpen(temporaryPath, 'chmod', fault), }), @@ -105,7 +103,7 @@ describe('writeAtomicFile', () => { await withTempDir(async (dir) => { const path = join(dir, 'settings.json'); const phases: string[] = []; - await writeAtomicFile(path, '{"a":1}\n', undefined, { + await writeAtomicFile(path, '{"a":1}\n', ownerOnlyFile, { open: async (temporaryPath, flags, mode) => { const handle = await open(temporaryPath, flags, mode); return { @@ -146,7 +144,7 @@ describe('writeAtomicFile', () => { const fault = new Error('dirsync failed'); await assert.rejects( () => - writeAtomicFile(path, '{"a":1}\n', undefined, { + writeAtomicFile(path, '{"a":1}\n', ownerOnlyFile, { syncDirectory: async () => { throw fault; }, @@ -169,7 +167,7 @@ describe('writeAtomicFile', () => { test('creates the target 0600 on POSIX', { skip: process.platform === 'win32' }, async () => { await withTempDir(async (dir) => { const path = join(dir, 'settings.json'); - await writeAtomicFile(path, '{}\n'); + await writeAtomicFile(path, '{}\n', ownerOnlyFile); assert.equal((await stat(path)).mode & 0o777, 0o600); }); }); @@ -181,35 +179,12 @@ describe('writeAtomicFile', () => { const path = join(dir, 'settings.json'); // A file created with a loose mode by an older writer. await writeFile(path, '{}\n', { encoding: 'utf8', mode: 0o644 }); - await writeAtomicFile(path, '{"a":1}\n'); + await writeAtomicFile(path, '{"a":1}\n', ownerOnlyFile); assert.equal((await stat(path)).mode & 0o777, 0o600); assert.equal(await readFile(path, 'utf8'), '{"a":1}\n'); }); }); - test('hardenDirectory creates a 0700 directory chain', { - skip: process.platform === 'win32', - }, async () => { - await withTempDir(async (dir) => { - const targetDir = join(dir, 'secrets', 'sub'); - await hardenDirectory(targetDir); - assert.equal((await stat(join(dir, 'secrets'))).mode & 0o777, 0o700); - assert.equal((await stat(targetDir)).mode & 0o777, 0o700); - }); - }); - - test('hardenDirectory re-chmods a pre-existing world-accessible directory to 0700', { - skip: process.platform === 'win32', - }, async () => { - await withTempDir(async (dir) => { - const loose = join(dir, 'loose'); - await mkdir(loose, { recursive: true, mode: 0o777 }); - await chmod(loose, 0o777); // mkdir's mode only applies on creation - await hardenDirectory(loose); - assert.equal((await stat(loose)).mode & 0o777, 0o700); - }); - }); - test('refuses to write through a pre-planted symlink at the temp path', { skip: process.platform === 'win32', }, async () => { @@ -222,7 +197,7 @@ describe('writeAtomicFile', () => { const plantedTemp = join(dir, '.credentials.json.planted.tmp'); await symlink(plantedTarget, plantedTemp); await assert.rejects( - () => writeAtomicFile(path, '{}\n', undefined, { randomUUID: () => 'planted' }), + () => writeAtomicFile(path, '{}\n', ownerOnlyFile, { randomUUID: () => 'planted' }), { code: 'EEXIST' }, ); assert.equal(await readFile(plantedTarget, 'utf8'), 'do not touch\n'); diff --git a/packages/storage/src/__tests__/settings-store-onboarding.test.ts b/packages/storage/src/__tests__/settings-store-onboarding.test.ts index 7dfac85efe..7a99587d7e 100644 --- a/packages/storage/src/__tests__/settings-store-onboarding.test.ts +++ b/packages/storage/src/__tests__/settings-store-onboarding.test.ts @@ -360,10 +360,11 @@ describe('SettingsStore.get file recovery', () => { } }); - it('writes settings.json owner-only (0600) and leaves no temp file behind', { + it('preserves a restrictive umask-derived settings.json mode and leaves no temp file behind', { skip: process.platform === 'win32', }, async () => { const workspaceRoot = await mkdtemp(join(tmpdir(), 'maka-settings-mode-')); + const previousUmask = process.umask(0o027); try { const store = createSettingsStore(workspaceRoot); @@ -372,10 +373,11 @@ describe('SettingsStore.get file recovery', () => { assert.deepEqual(await readdir(workspaceRoot), ['settings.json']); assert.equal( (await stat(join(workspaceRoot, 'settings.json'))).mode & 0o777, - 0o600, - 'settings.json carries plaintext credentials (bot secrets, proxy password)', + 0o640, + 'settings.json retains the mode produced by the legacy default and current umask', ); } finally { + process.umask(previousUmask); await rm(workspaceRoot, { recursive: true, force: true }); } }); diff --git a/packages/storage/src/__tests__/stable-storage.test.ts b/packages/storage/src/__tests__/stable-storage.test.ts index 8f4f715242..9d91e3bd3c 100644 --- a/packages/storage/src/__tests__/stable-storage.test.ts +++ b/packages/storage/src/__tests__/stable-storage.test.ts @@ -18,11 +18,23 @@ */ import assert from 'node:assert/strict'; -import { appendFile, lstat, mkdtemp, open, rename, rm, symlink, writeFile } from 'node:fs/promises'; +import { + appendFile, + chmod, + lstat, + mkdir, + mkdtemp, + open, + rename, + rm, + stat, + symlink, + writeFile, +} from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import test from 'node:test'; -import { readStableBoundedFile } from '../stable-storage.js'; +import { hardenDirectory, readStableBoundedFile } from '../stable-storage.js'; async function fixture(t: test.TestContext) { const directory = await mkdtemp(join(tmpdir(), 'maka-stable-file-')); @@ -99,3 +111,24 @@ test('rejects a pathname replaced after opening the file', async (t) => { /invalid stable file/u, ); }); + +test('hardenDirectory creates a 0700 directory chain', { + skip: process.platform === 'win32', +}, async (t) => { + const { directory } = await fixture(t); + const targetDir = join(directory, 'secrets', 'sub'); + await hardenDirectory(targetDir); + assert.equal((await stat(join(directory, 'secrets'))).mode & 0o777, 0o700); + assert.equal((await stat(targetDir)).mode & 0o777, 0o700); +}); + +test('hardenDirectory re-chmods a pre-existing world-accessible directory to 0700', { + skip: process.platform === 'win32', +}, async (t) => { + const { directory } = await fixture(t); + const loose = join(directory, 'loose'); + await mkdir(loose, { recursive: true, mode: 0o777 }); + await chmod(loose, 0o777); // mkdir's mode only applies on creation + await hardenDirectory(loose); + assert.equal((await stat(loose)).mode & 0o777, 0o700); +}); diff --git a/packages/storage/src/atomic-file-write.ts b/packages/storage/src/atomic-file-write.ts index 7cfc44f97e..f6a6320fb8 100644 --- a/packages/storage/src/atomic-file-write.ts +++ b/packages/storage/src/atomic-file-write.ts @@ -18,14 +18,14 @@ */ import { randomUUID } from 'node:crypto'; -import { chmod, mkdir, open, rename, rm } from 'node:fs/promises'; +import { open, rename, rm } from 'node:fs/promises'; import { basename, dirname, join } from 'node:path'; import { syncDirectory } from './stable-storage.js'; /** - * Owner-only atomic writer shared by the legacy settings, MCP, and credentials - * JSON stores. It publishes an exclusive hidden temp file with rename and - * removes that temp on failures before publication. + * Atomic writer shared by the legacy settings, MCP, and credentials JSON + * stores. It publishes an exclusive hidden temp file with rename and removes + * that temp on failures before publication. * * Durability support depends on the platform and storage stack: * - Linux and POSIX systems other than macOS: sync the temp file before @@ -36,17 +36,17 @@ import { syncDirectory } from './stable-storage.js'; * - Windows: sync the temp file before rename, but parent-directory sync is a * no-op because Node does not provide an equivalent directory fence. * - * File chmod policy is a documented invariant of this module, not a per-call - * choice: - * - file chmod: fail-loud on POSIX, skipped on Windows (no POSIX mode). - * Applies after the exclusive open so umask can never loosen the file. + * The caller owns the final file-mode policy. The writer applies that mode + * through the open handle before synchronization, fail-loud on POSIX and + * skipped on Windows (no POSIX mode). Callers may supply an exact private mode + * or one already derived from the process umask. * Directory creation and permission policy belong to each caller. */ export interface AtomicFileWriteOptions { - /** Mode for the final file. The temp is opened with it and re-chmod'd, so a - * pre-existing looser target is always tightened on the next write. */ - fileMode?: number; + /** Effective mode to apply to the temporary file before it is synchronized + * and published. */ + fileMode: number; } /** The fs surface `writeAtomicFile` needs; injectable for fault-injection @@ -82,11 +82,11 @@ export class AtomicFileWriteCommitUnknownError extends Error { export async function writeAtomicFile( path: string, contents: string, - options: AtomicFileWriteOptions = {}, + options: AtomicFileWriteOptions, dependencies: Partial = {}, ): Promise { const deps = { ...defaultDependencies, ...dependencies }; - const fileMode = options.fileMode ?? 0o600; + const { fileMode } = options; const tempPath = join(dirname(path), `.${basename(path)}.${deps.randomUUID()}.tmp`); // Only the entry this call created — and hasn't renamed away — may be // cleaned up. A pre-existing file or planted symlink the 'wx' open refused @@ -120,17 +120,3 @@ export async function writeAtomicFile( throw error; } } - -/** - * Create (or harden) an owner-only directory: recursive mkdir plus a - * fail-closed chmod, since mkdir's mode only applies on creation. Callers that - * store secrets use this before creating their file or update lock. - */ -export async function hardenDirectory(dir: string, mode: number = 0o700): Promise { - await mkdir(dir, { recursive: true, mode }); - if (process.platform === 'win32') { - await chmod(dir, mode).catch(() => {}); - return; - } - await chmod(dir, mode); -} diff --git a/packages/storage/src/credential-store.ts b/packages/storage/src/credential-store.ts index 8c34503f02..1998bba41c 100644 --- a/packages/storage/src/credential-store.ts +++ b/packages/storage/src/credential-store.ts @@ -19,8 +19,9 @@ import { readFile } from 'node:fs/promises'; import { dirname, join } from 'node:path'; -import { hardenDirectory, writeAtomicFile } from './atomic-file-write.js'; +import { writeAtomicFile } from './atomic-file-write.js'; import { withFileUpdateLock } from './file-update-lock.js'; +import { hardenDirectory } from './stable-storage.js'; /** * Pure-Node credential store. Shared by the desktop app and any diff --git a/packages/storage/src/mcp-config-store.ts b/packages/storage/src/mcp-config-store.ts index b8a1b84391..c4ae4f83b8 100644 --- a/packages/storage/src/mcp-config-store.ts +++ b/packages/storage/src/mcp-config-store.ts @@ -31,8 +31,9 @@ import { type McpServerConfig, type McpStdioServerConfig, } from '@maka/core/mcp'; -import { hardenDirectory, writeAtomicFile } from './atomic-file-write.js'; +import { writeAtomicFile } from './atomic-file-write.js'; import { withProcessLifetimeFileUpdateLock } from './process-lifetime-file-update-lock.js'; +import { hardenDirectory } from './stable-storage.js'; const MAX_SERVERS = 100; const MAX_ID_LENGTH = 128; diff --git a/packages/storage/src/settings-store.ts b/packages/storage/src/settings-store.ts index b180edbba8..94f549e310 100644 --- a/packages/storage/src/settings-store.ts +++ b/packages/storage/src/settings-store.ts @@ -192,11 +192,10 @@ class FileSettingsStore implements SettingsStore { private async write(settings: AppSettings): Promise { // SettingsStore does not own the workspace directory's permission policy: // sibling stores such as MCP config may independently harden the same root. - // Keep creation here plain; settings.json itself is 0600 because it carries - // plaintext credentials (bot secrets, proxy password). + // Keep both directory creation and the historical umask-derived file mode. await mkdir(dirname(this.settingsPath), { recursive: true }); await writeAtomicFile(this.settingsPath, JSON.stringify(settings, null, 2) + '\n', { - fileMode: 0o600, + fileMode: 0o666 & ~process.umask(), }); } diff --git a/packages/storage/src/stable-storage.ts b/packages/storage/src/stable-storage.ts index 45081ca9b1..50d515bf83 100644 --- a/packages/storage/src/stable-storage.ts +++ b/packages/storage/src/stable-storage.ts @@ -18,7 +18,7 @@ */ import { constants, type BigIntStats } from 'node:fs'; -import { lstat, open } from 'node:fs/promises'; +import { chmod, lstat, mkdir, open } from 'node:fs/promises'; import { dirname, isAbsolute, relative, resolve, sep } from 'node:path'; export interface ReadStableBoundedFileInput { @@ -99,6 +99,20 @@ export async function syncFile(path: string): Promise { } } +/** + * Create (or harden) an owner-only directory: recursive mkdir plus a + * fail-closed chmod, since mkdir's mode only applies on creation. Callers that + * store secrets use this before creating their file or update lock. + */ +export async function hardenDirectory(dir: string, mode: number = 0o700): Promise { + await mkdir(dir, { recursive: true, mode }); + if (process.platform === 'win32') { + await chmod(dir, mode).catch(() => {}); + return; + } + await chmod(dir, mode); +} + export async function syncDirectoryChain( path: string, root: string,