Fixing racy ensureDirSync broke ensureFileSync for tight permissions

[egfx-notifications]

I just updated our used version of deno std and the changes made to ensureDir and ensureDirSync in https://github.com/denoland/deno_std/pull/3242 break our unit tests. While I am in favor of fixing the race condition, I feel like one side effect has not been considered that could be easily solved (so I think, see proposal below).

The changes introduced in https://github.com/denoland/deno_std/pull/3242 make it so that ensureFile and ensureFileSync no longer work with write permissions limited to specific files, they would require write permissions to their parent directories now due to their internal usage of ensureDir and ensureDirSync

This can be easily tested with this example code (first two tests, rest is my proposed fix):

import { ensureFileSync as ensureFileSyncWithOldEnsureDirSync } from "https://deno.land/[email protected]/fs/mod.ts";
import {
  ensureDirSync,
  ensureFileSync as ensureFileSyncWithNewEnsureDirSync,
} from "https://deno.land/[email protected]/fs/mod.ts";
import {
  getFileInfoType,
  toPathString,
} from "https://deno.land/[email protected]/fs/_util.ts";
import * as path from "https://deno.land/[email protected]/path/mod.ts";

const testDirectory = "./tests";
const testFilepath = testDirectory + "/allowed-file";

ensureDirSync(testDirectory);

Deno.test(
  {
    name: "Works with old ensureDirSync",
    permissions: {
      read: [testDirectory],
      write: [testFilepath],
    },
  },
  () => {
    try {
      Deno.removeSync(testFilepath);
    } catch (err) {
      if (!(err instanceof Deno.errors.NotFound)) throw err;
    }
    ensureFileSyncWithOldEnsureDirSync(testFilepath);
  },
);

Deno.test(
  {
    name: "Does not work with new ensureDirSync",
    permissions: {
      read: [testDirectory],
      write: [testFilepath],
    },
  },
  () => {
    try {
      Deno.removeSync(testFilepath);
    } catch (err) {
      if (!(err instanceof Deno.errors.NotFound)) throw err;
    }
    ensureFileSyncWithNewEnsureDirSync(testFilepath);
  },
);

Deno.test(
  {
    name: "Would work with proposed ensureDirSync (but is it racy?)",
    permissions: {
      read: [testDirectory],
      write: [testFilepath],
    },
  },
  () => {
    try {
      Deno.removeSync(testFilepath);
    } catch (err) {
      if (!(err instanceof Deno.errors.NotFound)) throw err;
    }
    ensureFileSyncWithProposedEnsureDirSync(testFilepath);
  },
);

Deno.test(
  {
    name: "Proposed ensureDirSync also still works for directories",
    permissions: {
      read: [testDirectory],
      write: [testDirectory],
    },
  },
  () => {
    try {
      Deno.removeSync(testDirectory, { recursive: true });
    } catch (err) {
      if (!(err instanceof Deno.errors.NotFound)) throw err;
    }
    ProposedEnsureDirSync(testDirectory);
  },
);

Deno.test(
  {
    name:
      "Proposed ensureDirSync also still works for directories - throws on missing permission",
    permissions: {
      read: [testDirectory],
      write: [testDirectory],
    },
  },
  () => {
    try {
      Deno.removeSync(testDirectory, { recursive: true });
    } catch (err) {
      if (!(err instanceof Deno.errors.NotFound)) throw err;
    }
    Deno.permissions.revokeSync({ name: "write", path: testDirectory });
    ProposedEnsureDirSync(testDirectory);
  },
);

function ProposedEnsureDirSync(dir: string | URL) {
  try {
    Deno.mkdirSync(dir, { recursive: true });
  } catch (mkdirErr) {
    if (
      !(mkdirErr instanceof Deno.errors.AlreadyExists ||
        mkdirErr instanceof Deno.errors.PermissionDenied)
    ) {
      throw mkdirErr;
    }

    try {
      const fileInfo = Deno.lstatSync(dir);
      if (!fileInfo.isDirectory) {
        throw new Error(
          `Ensure path exists, expected 'dir', got '${
            getFileInfoType(fileInfo)
          }'`,
        );
      }
    } catch (lstatErr) {
      if (
        mkdirErr instanceof Deno.errors.PermissionDenied &&
        lstatErr instanceof Deno.errors.NotFound
      ) {
        throw mkdirErr;
      } else {
        throw lstatErr;
      }
    }
  }
}

// ensureFileSync from 0.185.0, just using the ensureDirSync proposed here
function ensureFileSyncWithProposedEnsureDirSync(filePath: string | URL) {
  try {
    // if file exists
    const stat = Deno.lstatSync(filePath);
    if (!stat.isFile) {
      throw new Error(
        `Ensure path exists, expected 'file', got '${getFileInfoType(stat)}'`,
      );
    }
  } catch (err) {
    // if file not exists
    if (err instanceof Deno.errors.NotFound) {
      // ensure dir exists
      ProposedEnsureDirSync(path.dirname(toPathString(filePath)));
      // create file
      Deno.writeFileSync(filePath, new Uint8Array());
      return;
    }
    throw err;
  }
}

Output

$ deno test --allow-read --allow-write minimal.test.ts
running 5 tests from ./minimal.test.ts
Works with old ensureDirSync ... ok (13ms)
Does not work with new ensureDirSync ... FAILED (5ms)
Would work with proposed ensureDirSync (but is it racy?) ... ok (5ms)
Proposed ensureDirSync also still works for directories ... ok (5ms)
Proposed ensureDirSync also still works for directories - throws on missing permission ... FAILED (5ms)

 ERRORS 

Does not work with new ensureDirSync => ./minimal.test.ts:35:6
error: PermissionDenied: Requires write access to "./tests", run again with the --allow-write flag
    Deno.mkdirSync(dir, { recursive: true });
         ^
    at Object.mkdirSync (ext:deno_fs/30_fs.js:118:7)
    at ensureDirSync (https://deno.land/[email protected]/fs/ensure_dir.ts:49:10)
    at ensureFileSync (https://deno.land/[email protected]/fs/ensure_file.ts:72:7)
    at file:///home/egf0s02d/projects/minion/minimal.test.ts:49:5

Proposed ensureDirSync also still works for directories - throws on missing permission => ./minimal.test.ts:89:6
error: PermissionDenied: Requires write access to "./tests", run again with the --allow-write flag
    Deno.mkdirSync(dir, { recursive: true });
         ^
    at Object.mkdirSync (ext:deno_fs/30_fs.js:118:7)
    at ProposedEnsureDirSync (file:///home/egf0s02d/projects/minion/minimal.test.ts:111:10)
    at file:///home/egf0s02d/projects/minion/minimal.test.ts:105:5

 FAILURES 

Does not work with new ensureDirSync => ./minimal.test.ts:35:6
Proposed ensureDirSync also still works for directories - throws on missing permission => ./minimal.test.ts:89:6

FAILED | 3 passed | 2 failed (82ms)

error: Test failed

As you can see I already included a proposed fix, that is changing the implementation from this https://github.com/denoland/deno_std/blob/aa57bf337bbf5e061752b92164b003a60e728951/fs/ensure_dir.ts#L47-L64

to this

export function ensureDirSync(dir: string | URL) {
  try {
    Deno.mkdirSync(dir, { recursive: true });
  } catch (mkdirErr) {
    if (
      !(mkdirErr instanceof Deno.errors.AlreadyExists ||
        mkdirErr instanceof Deno.errors.PermissionDenied)
    ) {
      throw mkdirErr;
    }

    try {
      const fileInfo = Deno.lstatSync(dir);
      if (!fileInfo.isDirectory) {
        throw new Error(
          `Ensure path exists, expected 'dir', got '${
            getFileInfoType(fileInfo)
          }'`,
        );
      }
    } catch (lstatErr) {
      if (
        mkdirErr instanceof Deno.errors.PermissionDenied &&
        lstatErr instanceof Deno.errors.NotFound
      ) {
        throw mkdirErr;
      } else {
        throw lstatErr;
      }
    }
  }
}

so if the directory creation fails due to missing write privileges, but it already exists, then we are fine, if it does not exist we just rethrow the original error.

This being a friday evening fix, there may be circumstances I have not considered yet, so I'll just leave that here to get some input.

• OS: Rocky Linux 8 + 9
• deno version: 1.32.5
• std version: 0.179.0+