[Bug] [copy] filtered-out items get statted, leading to unrecoverable exception

c-vetter commented 2 years ago

Operating System: Windows 10 Pro
Node.js version: 16.15.0
fs-extra version: 10.1.0

Reproduction

Try to copy(src, dest, { filter }, with:

src being a valid and accessible source directory, in my case a secondary drive's root
src having a child directory that is not accessible, in my case the System Volume Information directory that is owned by the OS and inaccessible to users
dest being a valid and accessible destination directory, in my case an empty directory on another drive
filter being a valid filter function that excludes the inaccessible directory, e.g. const filter = (fp)=>!fp.includes("System Volume Information")

Observed behavior

Error: EPERM: operation not permitted, lstat 'B:\System Volume Information'

Expected behavior

The copy operation proceeds without trying to access the filtered-out directory.

Additional information

During debugging, I tracked the issue to this line: https://github.com/jprichardson/node-fs-extra/blob/0220eac966d7d6b9a595d69b1242ab8a397fba7f/lib/copy/copy.js#L181

The way I understand the code, src and dest are stated before running the filter. I think that the filter should run before any attempt is made to stat any files.

If there are more important reasons why that should not happen, e.g. to enable #844, there should at least be a mechanism to recover from errors on a per-file basis in order to allow the copy operation to continue.

As it stands, I will have to work around this by giving a filtered set of files to copy 😢

RyanZim commented 2 years ago

How does fs.cp behave here?

c-vetter commented 2 years ago

How does fs.cp behave here?

Catastrophically, see https://github.com/nodejs/node/issues/44653

I don't have a Unix machine ready, so I cannot test there.

I tried docker, but was unable to reproduce a similar situation on account of having only root to play with.

RyanZim commented 2 years ago

At this point, we would like to maintain compatibility with fs.cp moving forward, so we'll generally follow however Node does it.

c-vetter commented 1 year ago

The initial issue seems to not be a bug, strictly speaking. Could be considered a docs issue.

Now that that's cleared up for the moment, I tried further and found fs.cp to behave the same way: https://github.com/nodejs/node/issues/44720

RyanZim commented 1 year ago

Fixed by Node in https://github.com/nodejs/node/pull/44922; we should backport.

RyanZim commented 1 year ago

I'm not sure the Node patch has tests that actually properly test this case. Any ideas how to construct a cross-platform test-case for this bug?

jprichardson / node-fs-extra