Bug: Uncaught throw on `EACCES` when calling `fs.readdirsync()`

[drazisil]

Environment

Node version: v18.8.0 npm version: v8.18.0 Local ESLint version: v8.23.0 Global ESLint version: Not found Operating System: linux 5.15.60-1-MANJARO

What parser are you using?

Default (Espree)

What did you do?

Attempt to scan a directory that conatined another directory with drw------- 3 root root for permissions. I was not root.

What did you expect to happen?

A warning or all other files and directories are silently parsed.

What actually happened?

The raw error EACCES: permission denied, scandir from the syscall bubbled up to onFatalError() https://github.com/eslint/eslint/blob/e098b5f80472e80c70603306e77e14ea15f1a93b/bin/eslint.js#L100 where it was displayed with no stack trace, making it very hard to locate where the syscall was occurring.

Participation

• [X] I am willing to submit a pull request for this issue.

Additional comments

https://github.com/eslint/eslint/blob/e098b5f80472e80c70603306e77e14ea15f1a93b/lib/cli-engine/file-enumerator.js#L147 throws an uncaught error on EACCES

I propose that EACCES be added as a condition, since ESLint has no access, it should not panic, it can behave as if the file doesn't exist, since it basically doesn't.

I will open a PR if there are no objections.

This is the root cause of https://github.com/eslint/eslint/issues/6498, I believe.

[mdjermanovic]

Hi @drazisil, thanks for the issue!

I propose that EACCES be added as a condition, since ESLint has no access, it should not panic, it can behave as if the file doesn't exist, since it basically doesn't.

Is this a common practice? If that is a directory that shouldn't be linted, then you could tell ESLint to ignore it by configuring ignorePatterns or .eslintignore, or by using --ignore-pattern on the command line.

Another thing to think about is how we could replicate this change (https://github.com/eslint/eslint/pull/16296) in the new FlatESLint class. In particular, in function findFiles where we are using globby. The only option to suppress EACCES seems to be suppressErrors: true, but that would suppress all other fs errors as well.

[drazisil]

I don't know if this is a common practice. Why does ESLint expect that a file not found is a normal error that should not be reported? I assumed that adding to it would be preferable to removing the code that eats the file not found error.

[drazisil]

@mdjermanovic Do you have additional feedback, or is https://github.com/eslint/eslint/pull/16296 blocked on me in some way?

[mdjermanovic]

I'm not sure if EACCES errors should be ignored, and how we would implement that in the new FlatESLint class, which will replace the current ESLint class and FileEnumerator at some point. A solution could be to add an option to ignore all fs errors, which would translate to globby's suppressErrors.

@eslint/eslint-tsc thoughts about this?

[drazisil]

bump. Would like this discussion to not be marked state, and I forget what the cutoff is for the bot.

[drazisil]

Can I get a comment from the TSC?

[nzakas]

@drazisil please be patient. We have over 100 issues and PRs to tend to so lower priority ones like this will be addressed once the higher priority ones are handled.

In general, I don’t like suppressing file system errors but I haven’t had the time to look closely at this yet.

[drazisil]

@drazisil please be patient. We have over 100 issues and PRs to tend to so lower priority ones like this will be addressed once the higher priority ones are handled.

In general, I don’t like suppressing file system errors but I haven’t had the time to look closely at this yet.

I'm sorry if I seemed impatient. I wasn't sure how the stale bot works. My apologies.

[btmills]

FWIW, at least on macOS, reporting an error and returning a non-zero exit code on denied access appears to have precedent in both ls and find:

$ sudo ls -lR
total 0
drwxr-xr-x  3 brandon  staff  96 Oct  2 20:05 mine

./mine:
total 0
drwx------  3 root  staff  96 Oct  2 20:06 notmine

./mine/notmine:
total 0
-rw-r--r--  1 root  staff  0 Oct  2 20:06 needle.txt
$ ls -lR
total 0
drwxr-xr-x  3 brandon  staff  96 Oct  2 20:05 mine

./mine:
total 0
drwx------  3 root  staff  96 Oct  2 20:06 notmine

./mine/notmine:
total 0
ls: ./mine/notmine: Permission denied
$ find . -name needle.txt
find: ./mine/notmine: Permission denied
$ sudo find . -name needle.txt
./mine/notmine/needle.txt

Given that, and assuming that pattern is mirrored by other tools, I'd lean against suppressing EACCESS. Either removing inaccessible resources from the tree or explicitly ignoring them would be my recommendation.

If reasonably easy to implement, I would support a better error message that reports which directory couldn't be accessed.

[drazisil]

Given that, and assuming that pattern is mirrored by other tools, I'd lean against suppressing EACCESS. Either removing inaccessible resources from the tree or explicitly ignoring them would be my recommendation.

Fair. I have closed my PR. :)

[btmills]

Makes sense! Would you be interested in working on a PR to improve the error message instead (or at least investigating whether that's practical)?

[drazisil]

Makes sense! Would you be interested in working on a PR to improve the error message instead (or at least investigating whether that's practical)?

I'll keep in in mind. I don't want to commit to a timeframe though, if someone else wants to get to it sooner. Could be a nice #hacktober project for someone :)

[github-actions[bot]]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.