node-fs-extra Line breaks in filenames cause infinite loops in mkdirs & mkdirsSync

Operating System: Windows 10 Enterprise 2016
Node.js version: 6.10.2
fs-extra version: 4.0.3

It would be nice to have error handling for line breaks in the mkdirs and mkdirsSync methods.

Correct names:

> require('fs-extra').mkdirs('dir_correct');
Promise { <pending> }

> require('fs-extra').mkdirs('dir_correct', (a) => console.log('callback', a));
undefined
> callback null

> require('fs-extra').mkdirsSync('dir_correct');
null

Directories with \n in paths are not created. It's okay, but here we get an infinite loop (at least in mkdirsSync), which is unacceptable in my opinion. In mkdirs with callback, the callback is not called.

> require('fs-extra').mkdirs('dir_incorrect\n');
Promise { <pending> }

> require('fs-extra').mkdirs('dir_incorrect\n', (a) => console.log('callback', a));
undefined

> require('fs-extra').mkdirsSync('dir_incorrect\n');
RangeError: Maximum call stack size exceeded
    at Object.fs.mkdirSync (fs.js:923:18)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:31:9)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:37:16)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:38:9)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:38:9)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:38:9)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:38:9)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:38:9)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:38:9)
    at mkdirsSync (C:\projects\test1\node_modules\fs-extra\lib\mkdirs\mkdirs-sync.js:38:9)

Dec 09 '17 18:12 wrager

Huh, these weird cases. What's the result of calling fs.mkdir with a newline in the filename? (when the parent directories exist)

Dec 09 '17 19:12 RyanZim

@RyanZim

> require('fs').mkdir('dir_incorrect\n');
undefined
> Error: ENOENT: no such file or directory, mkdir 'C:\projects\test1\dir_incorrect
'
    at Error (native)

Dec 09 '17 20:12 wrager

No wonder it crashes. ENOENT is also the error code we get when the parent doesn't exist, so we respond by trying to create the parent. That causes an infinite loop.

Not sure what's the best way to solve this, but we need to do something.

Dec 11 '17 15:12 RyanZim

What should happen when a newline character exists in the path? error out?

Dec 12 '17 20:12 manidlou

@manidlou That's the ugly part. AFAIK, on Windows, newlines aren't allowed. However, they may be allowed on some Linux systems, not sure.

We could either error out on a newline, or else put logic in place that if we get ENOENT after creating the parent, we just throw it. Not sure what's best.

Dec 13 '17 14:12 RyanZim

@manidlou @RyanZim It seems to me that you should delegate the handling of incorrect characters to the OS. Not only because operating systems are possible that allow line breaks in paths, but also because other similar characters are possible that we do not know about.

Dec 14 '17 08:12 wrager

@wrager I'm leaning in that direction as well. If only Windoze would throw a sensible error for invalid characters instead of ENOENT... :grimacing:

Dec 14 '17 15:12 RyanZim

@wrager yes delegating to the OS makes sense. @RyanZim you are right! there is inconsistency in error codes that OSes throw!

Not only that, there is a big discussion on what valid file paths should look like! There is this Wikipedia page on Filename on reserved words and characters in different OSes. IMO, dealing with checking for valid file path (that's what I had originally in my mind) is a mess because of noisy OS differences!

put logic in place that if we get ENOENT after creating the parent, we just throw it.

@RyanZim I think that's a good and feasible approach!

So then when that happens, we should remove the parent directory that was just created, right? Since essentially it is a failed operation?!

Dec 15 '17 07:12 manidlou

@manidlou Yeah, we probably should. That's gonna be messy, though. I didn't think about that.

Dec 15 '17 16:12 RyanZim

Well, hopefully https://github.com/nodejs/node/pull/21875 solves this for newer Node versions.

Aug 11 '18 22:08 RyanZim

Should be fixed since https://github.com/jprichardson/node-fs-extra/pull/894.

Oct 20 '22 19:10 RyanZim