search

jprichardson / node-fs-extra

Node.js: extra methods for the fs object like copy(), remove(), mkdirs()
MIT License
9.44k stars 776 forks source link

Remove unneeded and buggy stats check #976

Closed RyanZim closed 1 year ago

RyanZim commented 1 year ago

As per https://github.com/nodejs/node/pull/39372#discussion_r1001179295; haven't gotten a response from @bcoe there yet

Resolves https://github.com/jprichardson/node-fs-extra/issues/918

Hope this logic is right.

RyanZim commented 1 year ago

@manidlou @bcoe bump

RyanZim commented 1 year ago

I have been trying to write a test case for this; I cannot get to a place where this condition is ever reached. Do we need this at all? Or what am I missing?

manidlou commented 1 year ago

@RyanZim thank you for your efforts! I really appreciate it!

This is basically a wild edge case where we're trying to avoid broken copy when src is a link and its resolved path is inside the dest directory. We already have this test

https://github.com/jprichardson/node-fs-extra/blob/e6a95058c930953113177c9518f57e83cace3e79/lib/copy/__tests__/copy-prevent-copying-into-itself.test.js#L282-L296

so, what's wrong with this test?

RyanZim commented 1 year ago

@manidlou The problem is that that test doesn't actually exercise this condition. We'd expect the error to be of the format:

Cannot overwrite '${resolvedDest}' with '${resolvedSrc}'.

Instead, if you insert a log statement in that test, the actual error thrown is:

Error: Cannot overwrite directory '/tmp/fs-extra/copy-prevent-copying-into-itself/dest' with non-directory '/tmp/fs-extra/copy-prevent-copying-into-itself/src-symlink'.
RyanZim commented 1 year ago

I'm going to merge this as-is, without an explicit test, as it's certainly better than what we have now. However, we should still look into properly testing this later.

manidlou commented 1 year ago

Thank you @RyanZim! That makes sense.