fix(atomic): close descriptor mode gaps (#100)

Stress testing the atomic replacement surface found four defects.

Synchronous adapters without fchmodSync returned before the no-follow parent
identity check ran, so they wrote through symlinked parents. Validation now
always runs and only the mode application is conditional.

The copy fallback fsynced the disposable temp file rather than the published
destination, so a durability request did not cover the bytes that survived.
Ordering is corrected on both the async and sync paths.

A mode failure during the fallback could leave replacement bytes in place
despite restore-original; restoration now covers the original bytes and the
original mode.

Removing chmod and chmodSync from the injectable filesystem type broke fresh
strict-TypeScript adapter literals that still listed them. They are accepted
again as deprecated, unused compatibility fields.

Windows keeps explicit content, sync-order, publication, and descriptor-close
coverage where POSIX modes are not enforced, and the POSIX cases retain their
exact mode assertions.

Escalated separately: under umask 0777 a newly created directory lands at mode
000 and cannot be reopened for descriptor-bound mode application. Fixing that
needs descriptor-relative creation rather than a pathname chmod.

Author: steipete

CHANGELOG.md

@@ -4,6 +4,10 @@
 
 ### Security and Correctness
 
+- Keep the POSIX parent-directory no-follow identity check active for synchronous atomic-replacement adapters that omit `fchmodSync`, preventing the documented default adapter path from writing through a symlinked parent.
+- Synchronize the actual destination after descriptor-bound mode application when atomic rename uses copy fallback, and include both bytes and mode in bounded fallback restoration.
+- Continue accepting legacy atomic-replacement adapter literals that expose pathname `chmod` or `chmodSync` methods while keeping those methods unused.
+
 - Reject non-file sidecars without spinning, and read contended async and synchronous sidecar locks through bounded, no-follow, identity-checked descriptors; keep valid `createdAt` timestamps authoritative under filesystem clock skew; fail closed when fallback Windows ACL inspection returns no verifiable access entries; preserve synchronous stale-reclaim guards owned by another acquirer; and share one process-exit cleanup listener across file-lock manager domains.
 
 - Route `Root.copyIn()` and overwrite-capable `Root.write()` commits through a new descriptor-relative native replace rename, closing a parent-symlink swap that could create a missing destination directory outside the root before the JavaScript fallback detected the escape. Native `auto` and `require` mode now protect both create-only and replacing pinned writes; the explicitly best-effort JavaScript fallback remains available in `off` mode or when `auto` cannot load a binding.

docs/atomic.md

@@ -48,8 +48,8 @@ type ReplaceFileAtomicOptions = {
   copyFallbackRestore?: "restore-original" | "none"; // default: "none"
   maxRestoreBytes?: number;          // required with "restore-original"
   destinationHardlinks?: "reject"; // default unset (no destination nlink policy)
-  syncTempFile?: boolean;           // fsync(temp) before rename; default false
-  syncParentDir?: boolean;          // fsync(parent) after rename; default false
+  syncTempFile?: boolean;           // fsync(temp) before rename, or the final file after copy fallback; default false
+  syncParentDir?: boolean;          // fsync(parent) after rename, POSIX only; default false
   throwOnCleanupError?: boolean;    // report temp cleanup failure; default false
   beforeRename?: (params: { filePath: string; tempPath: string }) => Promise<void>;
   fileSystem?: ReplaceFileAtomicFileSystem; // injectable fs for tests
@@ -90,9 +90,10 @@ The default `copyFallbackRestore: "none"` preserves the existing fallback
 contract: a failed copy can leave a partial destination. For state files where
 preserving the old bytes is more important, choose `"restore-original"` and set
 an explicit `maxRestoreBytes` memory budget. If the destination exists, fs-safe
-snapshots it through a pinned descriptor, overwrites through that same
-descriptor, and synchronizes the result. Any write or sync failure triggers a
-restore and another sync through the same descriptor.
+snapshots it through a pinned descriptor, overwrites and mode-adjusts through
+that same descriptor, and synchronizes the result. Any write, mode, or sync
+failure triggers a byte-and-mode restore and another sync through the same
+descriptor.
 
 Restore failures are `FsSafeError("helper-failed")` values with typed
 `details.cleanup` set to `"restored"` or `"restore-failed"`. An original larger
@@ -163,7 +164,11 @@ Rename a path. If the rename fails with `EXDEV` (cross-device), fall back to
 copying into a staged sibling path, renaming that staged path into place, and
 then removing only the source entries that were copied. The fallback avoids
 buffering regular files into memory and does not tighten the destination parent
-directory mode.
+directory mode. Staged file modes are applied through their still-open handles.
+On POSIX, staged directory modes are applied through no-follow directory
+descriptors; on Windows, Node cannot portably open those descriptors and no
+pathname `chmod` fallback is attempted, so directory modes remain subject to
+Windows' `mkdir(mode)` behavior.
 
 ```ts
 import { movePathWithCopyFallback } from "@openclaw/fs-safe/atomic";
@@ -227,7 +232,7 @@ type ReplaceFileAtomicSyncFileSystem = {
 };
 ```
 
-The async interface already requires `open()`, whose `FileHandle` supplies `chmod()`, so injecting `node:fs` or another conforming adapter needs no new async member. On POSIX, that `open()` must support no-follow directory descriptors as Node does. A custom synchronous filesystem that passes `mode`, `dirMode`, or `preserveExistingMode` must supply `fchmodSync`; omission fails before any file or directory is created and never falls back to a pathname `chmod`. Existing synchronous adapters that request none of those options may omit it. Injecting plain `node:fs` supports explicit file and directory modes. Copy fallback applies the file mode through its pinned destination descriptor as well, preserving exact modes despite the process umask.
+The async interface already requires `open()`, whose `FileHandle` supplies `chmod()`, so injecting `node:fs` or another conforming adapter needs no new async member. On POSIX, that `open()` must support no-follow directory descriptors as Node does. A custom synchronous filesystem that passes `mode`, `dirMode`, or `preserveExistingMode` must supply `fchmodSync`; omission fails before any file or directory is created and never falls back to a pathname `chmod`. Existing synchronous adapters that request none of those options may omit it; their parent is still opened and identity-checked through a no-follow directory descriptor. Injecting plain `node:fs` supports explicit file and directory modes. Older adapter literals may continue to include `chmod` or `chmodSync` for source compatibility, but those operations are ignored. Copy fallback applies the file mode through its pinned destination descriptor as well, preserving exact modes despite the process umask.
 
 ## See also
 

src/replace-file-copy-fallback.ts

@@ -250,14 +250,18 @@ async function replacePinnedWithRestore(
   handle: FileHandle,
   replacement: Buffer,
   maxRestoreBytes: number,
+  replacementMode: number,
 ): Promise<void> {
+  const originalMode = (await handle.stat()).mode;
   const original = await readBounded(handle, maxRestoreBytes);
   try {
     await writeAll(handle, replacement);
+    await handle.chmod(replacementMode);
     await handle.sync();
   } catch (writeError) {
     try {
       await writeAll(handle, original);
+      await handle.chmod(originalMode);
       await handle.sync();
       throw restoreFailure(writeError, "restored");
     } catch (restoreError) {
@@ -274,14 +278,19 @@ function replacePinnedWithRestoreSync(
   fd: number,
   replacement: Buffer,
   maxRestoreBytes: number,
+  replacementMode: number,
+  fchmodSync?: (fd: number, mode: number) => void,
 ): void {
+  const originalMode = fsModule.fstatSync(fd).mode;
   const original = readBoundedSync(fsModule, fd, maxRestoreBytes);
   try {
     writeAllSync(fsModule, fd, replacement);
+    fchmodSync?.(fd, replacementMode);
     fsModule.fsyncSync(fd);
   } catch (writeError) {
     try {
       writeAllSync(fsModule, fd, original);
+      fchmodSync?.(fd, originalMode);
       fsModule.fsyncSync(fd);
       throw restoreFailure(writeError, "restored");
     } catch (restoreError) {
@@ -300,6 +309,7 @@ export async function copyFallbackReplace(params: {
   destinationHardlinks?: ReplaceFileDestinationHardlinkPolicy;
   restore: ReplaceFileCopyFallbackRestorePolicy;
   maxRestoreBytes?: number;
+  sync: boolean;
 }): Promise<void> {
   const sourcePreview = await params.fsModule.lstat(params.src);
   if (sourcePreview.isSymbolicLink() || !sourcePreview.isFile()) {
@@ -323,7 +333,12 @@ export async function copyFallbackReplace(params: {
       );
       if (pinned) {
         destHandle = pinned.handle;
-        await replacePinnedWithRestore(destHandle, replacement, params.maxRestoreBytes!);
+        await replacePinnedWithRestore(
+          destHandle,
+          replacement,
+          params.maxRestoreBytes!,
+          sourceStat.mode,
+        );
       }
     }
 
@@ -349,8 +364,11 @@ export async function copyFallbackReplace(params: {
         sourceStat.mode & 0o777,
       );
       await destHandle.writeFile(replacement);
+      await destHandle.chmod(sourceStat.mode);
+      if (params.sync) {
+        await destHandle.sync();
+      }
     }
-    await destHandle.chmod(sourceStat.mode);
   } finally {
     await destHandle?.close().catch(() => undefined);
     await sourceHandle.close().catch(() => undefined);
@@ -366,6 +384,7 @@ export function copyFallbackReplaceSync(params: {
   restore: ReplaceFileCopyFallbackRestorePolicy;
   maxRestoreBytes?: number;
   fchmodSync?: (fd: number, mode: number) => void;
+  sync: boolean;
 }): void {
   const sourcePreview = params.fsModule.lstatSync(params.src);
   if (sourcePreview.isSymbolicLink() || !sourcePreview.isFile()) {
@@ -394,6 +413,8 @@ export function copyFallbackReplaceSync(params: {
           destFd,
           replacement,
           params.maxRestoreBytes!,
+          sourceStat.mode,
+          params.fchmodSync,
         );
       }
     }
@@ -422,8 +443,11 @@ export function copyFallbackReplaceSync(params: {
         sourceStat.mode & 0o777,
       );
       writeAllSync(params.fsModule, destFd, replacement);
+      params.fchmodSync?.(destFd, sourceStat.mode);
+      if (params.sync) {
+        params.fsModule.fsyncSync(destFd);
+      }
     }
-    params.fchmodSync?.(destFd, sourceStat.mode);
   } finally {
     if (destFd !== undefined) {
       try {

src/replace-file-descriptor.ts

@@ -65,7 +65,7 @@ export function applyDirectoryModeSync(params: {
   mode: number;
   fchmodSync?: SyncFchmod;
 }): void {
-  if (process.platform === "win32" || !params.fchmodSync) {
+  if (process.platform === "win32") {
     return;
   }
 
@@ -74,7 +74,7 @@ export function applyDirectoryModeSync(params: {
   const fd = params.fsModule.openSync(params.dirPath, directoryOpenFlags());
   try {
     assertSameDirectory(expected, params.fsModule.fstatSync(fd), params.dirPath);
-    params.fchmodSync(fd, params.mode);
+    params.fchmodSync?.(fd, params.mode);
   } finally {
     params.fsModule.closeSync(fd);
   }

src/replace-file.ts

@@ -36,7 +36,10 @@ export type ReplaceFileAtomicFileSystem = {
     | "open"
     | "stat"
     | "lstat"
-  >;
+  > & {
+    /** @deprecated Accepted for adapter compatibility but never called. */
+    chmod?: typeof fs.chmod;
+  };
 };
 
 export type ReplaceFileAtomicSyncFileSystem = Pick<
@@ -58,6 +61,8 @@ export type ReplaceFileAtomicSyncFileSystem = Pick<
   | "readSync"
   | "writeSync"
 > & {
+  /** @deprecated Accepted for adapter compatibility but never called. */
+  chmodSync?: typeof syncFs.chmodSync;
   fchmodSync?: typeof syncFs.fchmodSync;
 };
 
@@ -123,6 +128,7 @@ async function renameWithRetry(params: {
   copyFallbackRestore: ReplaceFileCopyFallbackRestorePolicy;
   maxRestoreBytes?: number;
   destinationHardlinks?: ReplaceFileDestinationHardlinkPolicy;
+  syncFallback: boolean;
 }): Promise<ReplaceFileAtomicResult> {
   for (let attempt = 0; attempt <= params.maxRetries; attempt++) {
     try {
@@ -141,6 +147,7 @@ async function renameWithRetry(params: {
           destinationHardlinks: params.destinationHardlinks,
           restore: params.copyFallbackRestore,
           maxRestoreBytes: params.maxRestoreBytes,
+          sync: params.syncFallback,
         });
         return { method: "copy-fallback" };
       }
@@ -168,6 +175,7 @@ function renameWithRetrySync(params: {
   maxRestoreBytes?: number;
   destinationHardlinks?: ReplaceFileDestinationHardlinkPolicy;
   fchmodSync?: SyncFchmod;
+  syncFallback: boolean;
 }): ReplaceFileAtomicResult {
   for (let attempt = 0; attempt <= params.maxRetries; attempt++) {
     try {
@@ -187,6 +195,7 @@ function renameWithRetrySync(params: {
           restore: params.copyFallbackRestore,
           maxRestoreBytes: params.maxRestoreBytes,
           fchmodSync: params.fchmodSync,
+          sync: params.syncFallback,
         });
         return { method: "copy-fallback" };
       }
@@ -331,9 +340,9 @@ async function replaceFileAtomicUnserialized(
   const unregisterTempPath = registerTempPathForExit(tempPath);
   let tempExists = false;
   let originalError: unknown;
-  await fsModule.mkdir(dir, { recursive: true, mode: dirMode });
-  await applyDirectoryMode({ fsModule, dirPath: dir, mode: dirMode });
   try {
+    await fsModule.mkdir(dir, { recursive: true, mode: dirMode });
+    await applyDirectoryMode({ fsModule, dirPath: dir, mode: dirMode });
     tempExists = true;
     unregisterTempPath.setIdentity(await writeTempFile({
       fsModule,
@@ -356,6 +365,7 @@ async function replaceFileAtomicUnserialized(
       copyFallbackRestore: options.copyFallbackRestore ?? "none",
       maxRestoreBytes: options.maxRestoreBytes,
       destinationHardlinks: options.destinationHardlinks,
+      syncFallback: options.syncTempFile === true,
     });
     tempExists = false;
     unregisterTempPath();
@@ -404,9 +414,9 @@ export function replaceFileAtomicSync(
   const unregisterTempPath = registerTempPathForExit(tempPath);
   let tempExists = false;
   let originalError: unknown;
-  fsModule.mkdirSync(dir, { recursive: true, mode: dirMode });
-  applyDirectoryModeSync({ fsModule, dirPath: dir, mode: dirMode, fchmodSync });
   try {
+    fsModule.mkdirSync(dir, { recursive: true, mode: dirMode });
+    applyDirectoryModeSync({ fsModule, dirPath: dir, mode: dirMode, fchmodSync });
     tempExists = true;
     unregisterTempPath.setIdentity(writeTempFileSync({
       fsModule,
@@ -431,6 +441,7 @@ export function replaceFileAtomicSync(
       maxRestoreBytes: options.maxRestoreBytes,
       destinationHardlinks: options.destinationHardlinks,
       fchmodSync,
+      syncFallback: options.syncTempFile === true,
     });
     tempExists = false;
     unregisterTempPath();

test/atomic-dirmode-regression.test.ts

@@ -206,4 +206,26 @@ describe("atomic parent-directory descriptor modes", () => {
       await expect(fs.access(path.dirname(filePath))).rejects.toMatchObject({ code: "ENOENT" });
     },
   );
+
+  it.runIf(process.platform !== "win32")(
+    "rejects a symlinked parent with a sync adapter that omits fchmodSync",
+    async () => {
+      const root = await tempRoot("fs-safe-atomic-dirmode-symlink-");
+      const outside = await tempRoot("fs-safe-atomic-dirmode-outside-");
+      const linkedDir = path.join(root, "linked");
+      const filePath = path.join(linkedDir, "state.txt");
+      const { fchmodSync: _fchmodSync, ...syncWithoutFchmod } = fsSync;
+      await fs.symlink(outside, linkedDir, "dir");
+
+      expect(() => replaceFileAtomicSync({
+        filePath,
+        content: "must not escape",
+        fileSystem: syncWithoutFchmod,
+      })).toThrow("Atomic replace parent must be a real directory");
+
+      await expect(fs.access(path.join(outside, "state.txt"))).rejects.toMatchObject({
+        code: "ENOENT",
+      });
+    },
+  );
 });