diff --git a/.github/workflows/windows-recovery.yml b/.github/workflows/windows-recovery.yml index 5ffd068e0f..5662c2f37f 100644 --- a/.github/workflows/windows-recovery.yml +++ b/.github/workflows/windows-recovery.yml @@ -115,7 +115,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 d13667bfd7..4f2a9b9e48 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 | 18 | +| portable-candidate | 25 | | platform-contract | 31 | -Total Windows-excluded declarations: **76** +Total Windows-excluded declarations: **83** ## Inventory @@ -76,6 +76,10 @@ Total Windows-excluded declarations: **76** | 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` 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` | @@ -97,7 +101,10 @@ Total Windows-excluded declarations: **76** | 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` 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 new file mode 100644 index 0000000000..9427351753 --- /dev/null +++ b/packages/storage/src/__tests__/atomic-file-write.test.ts @@ -0,0 +1,249 @@ +/* + * 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 { + lstat, + 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 { + AtomicFileWriteCommitUnknownError, + 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-')); + 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', ownerOnlyFile); + 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 = join(dir, '.settings.json.fault.tmp'); + const fault = new Error(`${failurePhase} failed`); + await assert.rejects( + () => + writeAtomicFile(path, '{"a":1}\n', ownerOnlyFile, { + 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: process.platform === 'win32', + }, async () => { + await withTempDir(async (dir) => { + const path = join(dir, 'settings.json'); + const temporaryPath = join(dir, '.settings.json.fault.tmp'); + const fault = new Error('chmod failed'); + await assert.rejects( + () => + writeAtomicFile(path, '{"a":1}\n', ownerOnlyFile, { + 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', ownerOnlyFile, { + 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('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'); + await assert.rejects( + () => + writeAtomicFile(path, '{"a":1}\n', ownerOnlyFile, { + syncDirectory: async () => { + throw 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 its durability is not known to the caller. + assert.equal(await readFile(path, 'utf8'), '{"a":1}\n'); + assert.deepEqual(await readdir(dir), ['settings.json']); + }); + }); + + 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', ownerOnlyFile); + assert.equal((await stat(path)).mode & 0o777, 0o600); + }); + }); + + test('re-chmods a pre-existing world-readable target to 0600 on the next write', { + skip: process.platform === 'win32', + }, 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', ownerOnlyFile); + assert.equal((await stat(path)).mode & 0o777, 0o600); + assert.equal(await readFile(path, 'utf8'), '{"a":1}\n'); + }); + }); + + test('refuses to write through a pre-planted symlink at the temp path', { + skip: process.platform === 'win32', + }, 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. + const plantedTemp = join(dir, '.credentials.json.planted.tmp'); + await symlink(plantedTarget, plantedTemp); + await assert.rejects( + () => writeAtomicFile(path, '{}\n', ownerOnlyFile, { 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(plantedTemp)).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..7a99587d7e 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,26 @@ describe('SettingsStore.get file recovery', () => { await rm(workspaceRoot, { recursive: true, force: true }); } }); + + 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); + + 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, + 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 new file mode 100644 index 0000000000..f6a6320fb8 --- /dev/null +++ b/packages/storage/src/atomic-file-write.ts @@ -0,0 +1,122 @@ +/* + * 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 { open, rename, rm } from 'node:fs/promises'; +import { basename, dirname, join } from 'node:path'; +import { syncDirectory } from './stable-storage.js'; + +/** + * 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 + * 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. + * + * 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 { + /** 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 + * 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 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, + options: AtomicFileWriteOptions, + dependencies: Partial = {}, +): Promise { + const deps = { ...defaultDependencies, ...dependencies }; + 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 + // must not be deleted as "our" temp. + let tempCreated = false; + let published = 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); + published = true; + tempCreated = false; + await deps.syncDirectory(dirname(path)); + } catch (error) { + if (tempCreated) { + // Cleanup must never mask the original failure. + await rm(tempPath, { force: true }).catch(() => {}); + } + if (published) throw new AtomicFileWriteCommitUnknownError({ cause: error }); + throw error; + } +} diff --git a/packages/storage/src/credential-store.ts b/packages/storage/src/credential-store.ts index 325227e8d5..1998bba41c 100644 --- a/packages/storage/src/credential-store.ts +++ b/packages/storage/src/credential-store.ts @@ -17,10 +17,11 @@ * 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 { 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 @@ -242,67 +243,14 @@ 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. 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 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 hardenDirectory(dirname(path)); + await writeAtomicFile(path, contents, { fileMode: 0o600 }); } 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..c4ae4f83b8 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,7 +31,9 @@ import { type McpServerConfig, type McpStdioServerConfig, } from '@maka/core/mcp'; +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; @@ -195,30 +196,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..94f549e310 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 { + // SettingsStore does not own the workspace directory's permission policy: + // sibling stores such as MCP config may independently harden the same root. + // Keep both directory creation and the historical umask-derived file mode. 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: 0o666 & ~process.umask(), + }); } private withQueue(operation: () => Promise): Promise { 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,