fix(storage): unify atomic writes for JSON config stores

.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'

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'` |

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<T>(fn: (dir: string) => Promise<T>): Promise<T> {
+  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;
+  };
+}

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);
     });
   });

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');

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 });
+    }
+  });
 });