fs.cpSync / fs.cp / fs.promises.cp fails when src/dest args are Buffer

[jasnell]

Version

All

Platform

All

Subsystem

fs

What steps will reproduce the bug?

const { cpSync, cp, promises } = require('node:fs');

cpSync(Buffer.from('dirA'), Buffer.from('dirB'), { recursive: true, filter(...args) { console.log(...args); return true; } });  // throws

cp(Buffer.from('dirA'), Buffer.from('dirB'), { recursive: true, filter(...args) { console.log(...args); return true; } }, (err) => {
  console.log(err);  // errors
});

promises.cp(Buffer.from('dirA'), Buffer.from('dirB'), { recursive: true, filter(...args) { console.log(...args); return true; } });  // rejects

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

It should just work without throwing. The filter function should be called with the option to receive the path as either a string, Buffer, or URL.

What do you see instead?

It throws.

Additional information

The issue here is that the underlying implementation assumes that the paths are strings and tries to use the path.join function to concatenate them, which obviously does not work for Buffer and URL. The implementation has to be able to support Buffer and URL paths in order to properly support arbitrary text encodings in file names etc. Unfortunately, because of the way the callback is designed, it's not clear that there is an immediate non-breaking fix ready available.

@dario-piotrowicz @cjihrig @anonrig

[jasnell]

The immediately clear problem is this block in the copyDir function in the cp-sync.js file, specifically the two calls to path.join(...). The src and dest here might be Buffer or URL instances. Passing a URL just happens to work since a URL can be coerced to a string but it does so entirely by accident and can produce an invalid URL result since the name is not correctly appended to the URL base.

    while ((dirent = dir.readSync()) !== null) {
      const { name } = dirent;
      const srcItem = join(src, name);
      const destItem = join(dest, name);
      let shouldCopy = true;

      if (opts.filter) {
        shouldCopy = opts.filter(srcItem, destItem);
        if (isPromise(shouldCopy)) {
          throw new ERR_INVALID_RETURN_VALUE('boolean', 'filter', shouldCopy);
        }
      }

      if (shouldCopy) {
        getStats(srcItem, destItem, opts);
      }
    }

Unfortunately, however, changing this is going to be likely breaking since it would necessarily change the way the filter callback is invoked. I'm not sure if there's a reasonable way to fix this with the current callback design

[dario-piotrowicz]

Unfortunately, however, changing this is going to be likely breaking since it would necessarily change the way the filter callback is invoked. I'm not sure if there's a reasonable way to fix this with the current callback design

The only reasonable way to implement this in a non-breaky way I think would be to always convert everything to strings, basically keeping the exact same (incorrect but also fully documented) functionality we currently have but making sure that it works with Buffer objects as well (as a stop gap and then fix it in a breaky way in a major release).

Alternatively we could have an additional optional flag like "respectInputTypes" or something like that to signal whether inputs should or not be converted to strings like they currently are. However it'd seem pretty strange to me to have a flag to enable the correct behavior of an API... so I would avoid this and rather implement this in the way I mentioned above, or directly go with a breaky solution and release it in a major release (also since this has, as far as I can tell, been broken for years without huge consequences, I think that it's probably not something very time sensitive).

---

Putting the breaking aspect aside for a moment, can we simply implement URL/Buffer logic in the same way the current implementation works but:

• without converting the paths to strings
• replacing join calls with custom URL/Buffer concatenation logic?

Are there any particular nuances with this? or is it just a matter of taking what currently works on strings and implement equivalent logic based on different types?

[jasnell]

The only reasonable way to implement this in a non-breaky way I think would be to always convert everything to strings, basically keeping the exact same (incorrect but also fully documented) functionality we currently have but making sure that it works with Buffer objects as well (as a stop gap and then fix it in a breaky way in a major release).

Unfortunately converting everything to strings means likely breaking the paths when we don't know what character encoding they are using, so I don't think that works as a solution here.

There are two fundamental problems here:

1. The src and dest can be either a string, Buffer, or URL. The current code assumes it is always a string. When a URL is passed it ends up being coerced to a string but still concatenated incorrectly. When Buffer is used the Path.join(..) blows us because the string coercion with a Buffer does not work as transparently as it does with URL.
2. The second issue is that the individual names reported for directory entries are treated unconditionally as utf8 at the native layer, which is likely incorrect.

When then take both the src and dest and try combining them with name using path.join(...) which blows up.

I don't think there's a way of fixing this without a substantive breaking change. This gets quite tricky in the recursive case when Buffer or URL are used as the src or dest. I'm working through a proposed fix and will hopefully have a PR later today.

[jasnell]

To illustrate the problematic scenario here... most POSIX OS's treat file names as opaque byte strings with very few actual limitations. Generally, the only bytes that aren't allowed in a filename are known separators like ASCII / or ASCII \. So I can have a directory structure like:

/
  |- <directory name encoded as shift-js>
          |- <directory name encoded as utf16le>
                 |- <directory name encoded as latin1>
                      |- <filename encoded as utf8>

Now, if the node:fs implementation just ends up blindly converting these all into UTF8 strings it's going to fail spectacularly just trying to handle that first top level.

If we handle every thing as Buffer, then we can certainly concatenate the buffers using / as a delimiter, e.g. Buffer.concat([dir1, sep, dir2, sep, dir3, sep, file]) where sep is equal to Buffer.from(path.sep), and we'd be able to work with that. However, if the path is given as a URL that gets a bit tricky as we currently don't have a URL-to-Buffer conversion for file paths... we probably need one.

And, fwiw, this is not a hypothetical case. This is exactly the kind of issue I was helping a customer solve back when I added the ability to represent file paths as Buffer in the first place.

I believe the only way to adequately fix this will be to come up with an entirely new signature for the filter method. Essentially, runtime deprecate options.filter and add a new alternative like options.predicate that uses a signature that adequately handles this type of case. I'm still working out what the signature of that options.predicate should be.

[jasnell]

Another challenge in this... our current file URL to path logic does not work when the file URL has to percent encode the file name in a different encoding.. that is, the conversion is assuming that the percent encoding is of utf8 bytes... When it is, for instance, shift-jis bytes, it blows up... In the example screenshotted below, the percent encodings align with the actual file name on disk as shift-jis.

[jasnell]

Ok, here's a proposed set of fixes:

1. When the path is specified as a URL, we will convert that into a Buffer instead of a string. This will require a new alternative implementation of the ul.fileURLToPath(n) method. url: add fileURLToPathBuffer API #58700
2. When the path is specified as a string, we will convert that into a Buffer assuming utf8 encoding.
3. We have the readdir operation always return the child paths as Buffer instances. We can then join those using Buffer.concat rather than path.join.
4. The new options.predicate would receive Buffer instances for the src and dest instead of strings, with the existing options.filter being runtime deprecated.