sindresorhus / globby

User-friendly glob matching
MIT License
2.53k stars 129 forks source link

`.gitignore: true` makes globby crash on directory with bad read permissions #254

Closed ChristophP closed 7 months ago

ChristophP commented 1 year ago

In our project we have a folder under the root folder which is .gitignored and used by a docker image to write to database data. Since it's running inside a Docker container the directory has a different owner and the files can not be read by the user outside of the docker container.

When running globby with the gitignore: true option it crashes.

Steps to reproduce

mkdir someproject
cd someproject
npm init -y && npm i globby
mkdir testdir
chmod -r testdir # remove read access
node --input-type=module -e "const { globby } = await import('globby'); console.log(await globby('test', { gitignore: true }))"

The last line crashes with an error like this:

[Error: EACCES: permission denied, scandir '/Users/d441365/projects/deedop/freigeist/someproject/testdir'] {
  errno: -13,
  code: 'EACCES',
  syscall: 'scandir',
  path: '/Users/d441365/projects/deedop/freigeist/someproject/testdir'
}

Setting gitignore: false makes the error go away

It's not globby's fault that it can't read the directory. Would you consider adding a way to alter globby's behavior in this case though? For example with an option like unreadable: 'skip' which would simply skip directories that can't be read.

PS the whole thing came up when using knip which is using globby under the hood https://github.com/webpro/knip/issues/172

sindresorhus commented 1 year ago

I would say this is a bug. The gitignore: true functionality should simply ignore a directory if it's not readable as the globbing would not find anything there anyway.

ChristophP commented 1 year ago

Yes, I agree. Ignored und readable directories would be a sensible default behavior. Maybe along with a warning that a certain directory could not be read.

ChristophP commented 1 year ago

I started looking into this a bit.

To me the best way to fix this would be to pass suppressErrors as true always in this line.

https://github.com/sindresorhus/globby/blob/main/ignore.js#L68

That even seems to be intented according to the fast-glob README.

Can be useful when the directory has entries with a special level of access.

However, I am not sure if this has unwanted side effects such as other errors that users would want to know about not being reported.

Also, this might mean that it's worth mentioning in the globby README that suppressErrors is true by default.

Do you think this would be a solution worth implementing or not?

The actual implementation could probably be as simple as changing the default value of suppressErrors to true if it's not passed in this line

const normalizeOptions = (options = {}) => ({
    cwd: toPath(options.cwd) || process.cwd(),
-   suppressErrors: Boolean(options.suppressErrors),
+       suppressErrors: options.suppressErrors === false ? false : true, // make sure supressErrors is aways true unless explicitly passed as false
    deep:
        typeof options.deep === "number" ? options.deep : Number.POSITIVE_INFINITY,
});
florent-andre commented 11 months ago

:+1: {suppressErrors:true} as option remove the error on Linux for a drwx------ 4 root root folder

phiresky commented 11 months ago

It seems like the underlying issue here is more that globby indiscriminately recursively searches for .gitignore files everywhere before doing anything else, which seems to mean it scans through all sorts of directories that are deeply nested inside dirs that are gitignored, which eem like a large perf issue in addition to causing this read permission error (i don't want it to scan 1 million files in my gitignored node_modules folder just because there might be a gitignore file somewhere in there