feat(fs): undo deprecation of exists and add permission and type check to it

[martin-braun]

Introduces readable Undos the deprecation and enhances exists of the fs module.

exists was deprecated due to TOCTOU issues, but there are valid use cases to have such function:

• Checking for path accessibility prior using third-party tools that rely on such given path which can potentially avoid spawning expensive processes for no reason
• Checking access rights on the given path without locking it through opening or handling exceptions with try / catch
• Avoiding unnecessary per-project user abstractions of Deno.lstat / Deno.stat to compensate the deprecation of exists

Improvements compared to the former exists:

• The former exists was returning false on Windows systems if the file was not readable, but it was returning true on POSIX systems (with exceptions, even if the file was not readable, this is not the case anymore: Options were added that provide an isReadable flag to effectively check if the file can be read as well, for Windows and POSIX systems
• Since in almost every case the user either expect a directory or a file (but not both) the flags isDirectory and isFile were added to the options to address those cases, otherwise the user would still rely on manually calling Deno.lstat / Deno.stat afterwards which would defeat the purpose of this implementation and introduce more TOCTOU / race conditions
• All implementations abstract from Deno.stat instead Deno.lstat to follow symlinks and ensure the option flags are working properly on links as well

References:

• The problem originated from BREAKING(fs): deprecate exists and existsSync #1364
• The solution was discussed in detail at #2102 which was inspired by post-reactions at bug(fs): fs.exists/fs.existsSync throws when querying subpath of existing file. #1216

cc @lambdalisue @twome @jespertheend @dionjwa @dev-nicolaos

[CLAassistant]

All committers have signed the CLA.

[iuioiua]

This PR would also close #2594

[iuioiua]

You might want to double-check the code examples in the separate functions as they seem to incorrectly use readable instead of the corresponding function.

[martin-braun]

I would also like to feature tests that involves an existing file / directory where the user has no read-permission on, but the current way is not very suitable. I can commit a file or directory that I cannot read, obviously.

To include those tests, I'd need to create a file and directory, remove their user permissions, execute the test, add user permissions back and remove the file and directory again.

Is this overkill? Is this recommend? @bartlomieju @kt3k

[martin-braun]

You might want to double-check the code examples in the separate functions as they seem to incorrectly use readable instead of the corresponding function.

Maybe it's too late for me, could you please comment on the affected lines, directly?

[iuioiua]

I like this. My concerns, neither of which, I have strong opinions on:

1. Would readableSymlink also be something we want?
2. I think function names like isReadable, isReadableDir, etc. would be clearer to the developer.

[martin-braun]

@iuioiua Thank you, I was missing the comments, copy/paste caught me here.

1. Would readableSymlink also be something we want?

Please have a look at our final posts in the discussion thread.

Since we abstract from stat instead of lstat, symlinks will be followed. The usage of readable* should default to a behavior that is expected to the user.

readable* should primarily be used to ensure a file or directory is available for any third-party process. If the Deno script needs to work with the file or directory directly, it should access it directly and thus not use readable* at all to prevent race-conditions.

In my experience, most tools follow symlinks, so should readable* in my honest opinion.

A user might create a symlink to have a specific folder or file in another location (i.e. to sync it with the cloud). A Deno script should not break due to this.

So, we abstract from stat not lstat, a readableSymlink would imply that readableDir or readableFile do not follow symlinks, thus getting inconsistencies.

Checking a symlink is such a rare occurrence that we would prefer the user falls back to an own lstat implementation. readable* has been created to fulfill just a common case, thus I think readableSymlink is not necessary.

2. I think function names like isReadable, isReadableDir, etc. would be clearer to the developer.

I'm not sure. I tried to follow existing code, there is no such file starting with is_ and readable is an adjective term. exists was also named exists and not doesExist, but I have no strong opinion about it.

[kt3k]

I think function names like isReadable, isReadableDir, etc. would be clearer to the developer.

I agree with this opinion. We already use the word readable for ReadableStream in several places (for example FsFile.readable is ReadableStream of its contents. ref. https://doc.deno.land/deno/stable/~/Deno.FsFile ). So this usage of readable sounds confusing to me.

[iuioiua]

Yeah, when I first saw the PR title, I thought it was related to streams 😅

[martin-braun]

I'm currently rewriting the tests to get rid of usage of testdata files and to be able to include permission tests and to my surprise there was some information that caught me off-guard:

2. "Exists" is a nebulous concept. node -p 'fs.existsSync("/root/.ssh/id_rsa")' prints false even though the file exists on my system. It's there, it's just not accessible.

Thus, I falsy assumed that Deno.stat is unable to access a file without read permissions, which is not the case. I'm not sure about Linux, but on macOS this test will fail:

import * as asserts from "https://deno.land/std@0.160.0/testing/asserts.ts";

async function isReadable(path: string | URL): Promise<boolean> {
  try {
    await Deno.stat(path);
    return true;
  } catch (error) {
    if (error instanceof Deno.errors.NotFound) {
      return false;
    }
    throw error;
  }
}

Deno.test("failing test", async function () {
  const tempDirPath = await Deno.makeTempDir();
  const tempFilePath = path.join(tempDirPath, "0.ts");
  await Deno.create(tempFilePath);
  try {
    await Deno.chmod(tempFilePath, 0o000);
    asserts.assertEquals(await isReadable(tempFilePath), false); // exists, but not readable
  } catch (e) {
    throw e;
  } finally {
    await Deno.remove(tempDirPath, { recursive: true });
  }
});

So it seems a file that a user cannot read its contents from can still be accessed with Deno.stat. Unfortunately, FileInfo.mode is unstable which makes it unacceptable to alter isReadable with a permission check. I don't really think refactoring to exists, existsDir and existsFile would be a satisfying solution, but at this point I see no other way.

Even when I was able to check the permissions, it would rather encourage a re-implementation of node's fs.access instead.

I'm seeking some advice at this point.

[jespertheend]

I'll have to admit I also expected Deno.stat("/root/.ssh/id_rsa") to reject.
Fwiw, FileInfo.mode is unstable, but it does not require the --unstable flag. So I'm not sure how unacceptable a permission check would really be.

To be honest, I think returning true in the case of failing permissions would be fine for most use cases. But then isReadable* wouldn't be the correct name anymore. That would bring us back into exists() territory which is less ideal I think.

[martin-braun]

@jespertheend It's good to know that FileInfo.mode doesn't require the --unstable flag. The description probably says it's unstable, because it's null on Windows, Deno.chmod doesn't work properly on Windows as well (see denoland/deno#4357).

I booted Windows, created two files (test.txt and test2.txt) in C:\ProgramData. I created a new user testuser and denied any permissions to that user for test.txt:

Now I launched deno as testuser with runas /user:"testuser" deno and tried to access both files with this result:

> Deno.stat("C:\\ProgramData\\test.txt")
Promise {
  <rejected> PermissionDenied: Access is denied. (os error 5), stat 'C:\ProgramData\test.txt'
    at async Object.stat (deno:runtime/js/30_fs.js:314:17)
}
> Deno.lstat("C:\\ProgramData\\test.txt")
Promise {
  <rejected> PermissionDenied: Access is denied. (os error 5), stat 'C:\ProgramData\test.txt'
    at async Object.lstat (deno:runtime/js/30_fs.js:297:17)
}
> Deno.stat("C:\\ProgramData\\test2.txt")
Promise {
  {
    isFile: true,
    isDirectory: false,
    isSymlink: false,
    size: 0,
    mtime: 2022-10-18T18:16:46.311Z,
    atime: 2022-10-18T18:16:46.311Z,
    birthtime: 2022-10-18T18:16:46.311Z,
    dev: null,
    ino: null,
    mode: null,
    nlink: null,
    uid: null,
    gid: null,
    rdev: null,
    blksize: null,
    blocks: null
  }
}

So, on Windows stat and even lstat fail to access the restricted file with a Deno.errors.PermissionDenied error. So when catching this error and returning false the function name isReadable* is very correct here.

On other systems stat probably will not fail when the file cannot be read, but FileInfo.mode should not be null on POSIX systems, thus allow us to additionally check if the file is readable.

On Windows I tested the same with directories and the behavior is the same to files.

So in favor of consistent behavior, we should include the permission check, since it will raise an error on Windows anyways. I thought about catching the Deno.errors.PermissionDenied and check the value of FileInfo.mode if it's not null.

If I could get the current user ID and group ID, I could compare it with FileInfo.uid and FileInfo.gid to see how I have to parse FileInfo.mode properly. Deno.getUid() and Deno.getGid() are unstable and require the --unstable flag.

So I can really only ensure it's readable by reading the directory / opening the file, which I don't like, ... or I catch Deno.errors.PermissionDenied on Windows and return true, while refactoring back to exists*, so the function would always return true if the directory or file exists, even it cannot be read.

I really want consistent behavior across the platforms.

[jespertheend]

Would it make sense to release without the permissions check first and then add it in a follow up PR once Deno.getUid() and Deno.getGid() are stable? Otherwise reading the file sounds less than ideal, but could also work as a temporary solution. I can't think of any other way.

[martin-braun]

@jespertheend It would be possible, although my biggest concern is the changing behavior of the implementation between the platforms and between concurrent releases. I really want to thrive for a better solution.

I committed an implementation (incl. tests) that relies on the --unstable flag until #2791 is fixed. The implementation is complete, the tests lack some more test cases:

• Permission checks don't include directory / file checks with items that you are not owner of, it's probably overkill given the simplicity of the implementation, especially since the tests should not create users / groups on your system temporary
• I'm unable to test symbolic links whose index (the link itself) has missing permissions (only relevant for MacOS and a few other BSD variants, where this is possible through chmod -h), the issue boils down to libc (which is used by Rust, which is used by Deno) not allowing to modify symlink permissions. (I poked the Rust team about it, it might works to set those with Deno.run directly, though.)

Whatsoever, these issues are neglectable, but should be mentioned.

EDIT: There is no check on links without permissions necessary. MacOS is still able to follow links without read permission, it seems the read permission is only necessary for tools like ls and readlink, but the symbolic link can always be dereferenced:

$ echo OK > test
$ ln -s test test-link
$ sudo chmod -h 000 test-link
$ ls -hal
total 2
-rw-r--r--  1 marty  staff     3B Oct 19 04:38 test

ls: ./test-link: Permission denied
l---------  1 marty  staff     4B Oct 19 04:38 test-link
$ readlink test-link
$ cat test-link
OK

So this is not even a concern anymore. :)

[martin-braun]

I've combined isReadable into one. In regards of exists to address cases in which it is necessary to know if a file/directory exists without caring for its permissions, I am refactoring isReadable to exists, but providing an isReadable flag on the options to further reduce redundancy.

[martin-braun]

@jespertheend Please do and thanks in advance. My Parallels license has expired and I currently have limited time to setup a new Windows VM in a different system. I also have disk issues at the moment (limited space, errors when trying to expand its space) as well as I couldn't get UTM (QEMU) to work in a short time. Would be good if I could focus on all that later.

Basically, you want to deno test --allow-all --unstable --filter "[fs] exists" fs/exists_test.ts while slowly commenting out each sub test to figure out which test fails and why it does.

[jespertheend]

@martin-braun It seems like the there are two issues:

• Deno.chmod not being supported, causing NotSupported: The operation is not supported to be thrown. These can be fixed by wrapping all of the Deno.chmod calls in the finally blocks with the same if (Deno.build.os !== "windows") statement as is used above it.
• Deno.symlink throws Error: A required privilege is not held by the client. I'm not sure if ci is being run with the right privileges, if so I don't think this should be an issue. If I run deno test in cmd as administrator I don't run into this error.

If you want I can commit the if statements to fix the first issue. Though I think you would have to give me write access to your fork, so it might be easier to just add these yourself. There are 6 places in total that need this.

[martin-braun]

@jespertheend First of all, thanks a lot. The first issue was an easy fix, thanks to your help. I guess I forgot about these chmod calls in finally.

In regards of the 2nd issue: Could you please check the contents of tempLinkDirPath, please? It seems the path that is returned from Deno.makeTempDir() is not writable by the current user on Windows machines. Since it works as Admin, I assume the --allow-write is not missing during these tests. Maybe, you could also check if

const tempDirPath = await Deno.makeTempDir({ prefix: 'deno_exists_test_' });

fixes the issue?

@iuioiua Is this a known limitation of the Windows test machine or even the Windows OS? If it is, I could bypass this by using a relative directory instead, although I remember we wanted to get rid of the test-data directory in the long term.

[jespertheend]

@martin-braun The folder is completely empty. Or at least that's the case for the 'existsDirLink' test. The prefix only adds a prefix to the name of the directory, but it doesn't change the location so it still runs into the same issue.

[jespertheend]

Hey I think this should be good to go once the next workflow runs.
I created a PR of this branch on my own fork in order to make the workflow run and all tests seem to pass.

[martin-braun]

Great to see the Windows test passed, but lint failed due to something unrelated.

error: request or response body error: error reading a body from connection: connection reset
    at https://deno.land/std@0.177.0/node/internal_binding/_libuv_winerror.ts:26:28
Error: Process completed with exit code 1.

[kt3k]

Core team discussed this last week and now we are in favor of landing this. Thank you for your effort and discussions for long time

[kt3k]

LGTM

[iuioiua]

[lambdalisue]

🎉

[lino-levan]

🎉🎉🎉 Nice work! Glad it landed.

[martin-braun]

I'm glad we made it eventually. :)
Thanks for your patience to everyone. 🙏

[twome]

Thank you so much for all your work!