Node: path.posix.resolve, path.posix.relative return wrong path on Windows

[DuanPengfei]

• Version: v8.1.1
• Platform: Windows 10
• Subsystem: path

path.posix.resolve, path.posix.relative return wrong path on Windows.

For example:

path.posix.resolve('a/b/c');  // returns: 'C:\\Users\\fei\\Desktop\\share\\node/a/b/c'

Ref: https://github.com/nodejs/node/issues/13683

[DuanPengfei]

Hope I can find a good way to fix it. :joy:

[refack]

I had an idea, add a new "strict mode" to path:

const path = require('path');
path.posix.resolve('a/b/c');  // returns: 'C:\\Users\\fei\\Desktop\\share\\node/a/b/c';
path.strict = true;
path.posix.resolve('a/b/c');  // throws

[seishun]

Can you clarify why it's wrong?

[refack]

Can you clarify why it's wrong?

path.posix.resolve('a/b/c');  // returns: 'C:\\Users\\fei\\Desktop\\share\\node/a/b/c';

The result is not a valid path nor on windows (the host) and definitely not on POSIX (the expected target).
The root cause is that resolve uses process.cwd() which is the only point where the path module does something that is not pure string manipulation.

See discussion at: https://github.com/nodejs/node/issues/13683#issuecomment-309014169

[tniessen]

I am against introducing a strict mode. Instead, I would prefer to always throw if process.cwd() needs to be used and the host platform is not the same as the API platform. If we care about breakage too much, we can introduce a deprecation cycle such that path.posix.resolve and path.win32.resolve will print warnings instead of throwing.

I could put together a PR if this gets approval.

[refack]

I am against introducing a strict mode. Instead, I would prefer to always throw if process.cwd() needs to be used and the host platform is not the same as the API platform. If we care about breakage too much, we can introduce a deprecation cycle such that path.posix.resolve and path.win32.resolve will print warnings instead of throwing.

@tniessen I had the same opinion, but @mscdex and @addaleax are strongly against. The point they both raised is that this is not strictly a bug but a weird behavior that people are used to, and throwing will block "valid" code paths. With that in mind, strict mode would allow us in the future change the default behavior while allowing opting-out by setting path.strict = false;

[tniessen]

Introducing a global setting for such a "strict mode" sounds like an unnecessary side effect to me (unless you want to make that setting per module or similar), e.g. if an application enables strict mode and a dependency expects non-strict mode or vice versa. If you really want to implement different modes, I would suggest require('path').strict().

I would like to include @addaleax in this discussion so we don't have to split it and continue parts of it in #13683.

Throwing an error instead of fixing the bug seems like a really bad idea; I think there is no reason inherent to the API why path.posix.relative('a/b/c', '../../x') wouldn’t be able to return a correct result on Windows.
(https://github.com/nodejs/node/issues/13683#issuecomment-309206372)

I understand that; but that should work just fine on Windows, no?
(https://github.com/nodejs/node/issues/13683#issuecomment-309207118)

Let's look at something simple:

path.posix.resolve('foo/bar')

So what would be the "correct result" on Windows? Let's say process.cwd() is F:\node. Would you expect F:\node/foo/bar? Not a good idea, it is not a valid posix path. How about F:/node/foo/bar? Looks good, but resolve must return an absolute path, and path.posix.isAbsolute('F:/node/foo/bar') is false, and there is no way to change that, as this is a relative path on posix systems. How about /F:/node/foo/bar? Too bad, now it is not even a valid path on Windows anymore.

[XadillaX]

how about starting a vote?

[addaleax]

I also really don’t like the idea of strict mode; all path functions currently are pure, up to their dependency on cwd(), and that’s a good thing. If you want it, add an extra parameter or add new methods, don’t introduce global mutable state.

Re: always throwing when process.cwd() is used… people are doing weird stuff with path, and the thing is, the end result isn’t always dependent on all of the input. For example, one could use path.resolve() on two different paths, where the cwd may or may not be used, and then call path.relative() and get a result that doesn’t actually depend on the cwd or only parts of it, even though the intermediate results do.

It’s a nice thing that explicit path.posix.resolve() is likely used much less frequently than just path.resolve(); but I am afraid users might just use that e.g. if they need URL-like paths for some reason, or basically just “want to get forward slashes” for any possible reason, rather than using the Windows method and then fixing up the result to fit their format.

How about F:/node/foo/bar? Looks good, but resolve must return an absolute path, and path.posix.isAbsolute('F:/node/foo/bar') is false

I would accept that; it’s strictly better than the current state of things, and even though path.posix.isAbsolute('F:/node/foo/bar') returns false, it is still an absolute path for all other purposes.

how about starting a vote?

Our process is consensus-seeking, so usually things don’t work that way; we’ll keep discussing the issue (and maybe coming up with new solutions) until we either reach consensus or it becomes clear that we won’t reach consensus. I don’t think we’re at either point yet.

If we won’t be able to get consensus, we’ll likely put this on the CTC meeting agenda, where it will be discussed, and we’ll only vote if the CTC won’t get consensus either. I think it’s unlikely that it will come to that.

[tniessen]

don’t introduce global mutable state.

Definitely +1!

and the thing is, the end result isn’t always dependent on all of the input

Good point, we might need to come up with a better solution.

I would accept that; it’s strictly better than the current state of things, and even though path.posix.isAbsolute('F:/node/foo/bar') returns false, it is still an absolute path for all other purposes.

Then let's have a look at path.posix.resolve(process.cwd(), 'foo'). Everything is fine when executed under linux, but it will return something like 'F:\\node/F:\\node/foo' on Windows, and there is no way to change that, because the posix API considers the result of process.cwd() relative.
Sure, you could say that it does not make sense to do this, using a Windows API call with the posix API... But that's exactly what we are doing in the current path module.

If we decide not to throw an Error in these situations, we should at least add notes to the docs explaining the current behavior (after fixing bugs as in #13683).

[refack]

don’t introduce global mutable state.

Definitely +1!

Offf I forgot about the globalness of modules... How about something like...

const path = require('path');
path.strict.posix.resolve('.\\gaga') // throws
const { strict: spath } = path;
spath.posix.resolve('.\\gaga') // throws

Again it's opt-in, and gives us a path to deprecation with explicit and opt-out in the future.

[refack]

Another idea: an implementation that's pure strings, with an optional cwd as arg

const { Pure } = require('path');
const purePath1 = new Pure('C:\\fakepath')
purePath1.resolve('gaga') === 'C:\\fakepath\\gaga';
const purePath2 = new Pure();
purePath1.resolve('gaga') === Pure.resolve('gaga') // throws or returns just 'gaga'

[refack: tweaked code]

[DuanPengfei]

I also prefer the pure function, but if this is done, does it mean that the history code needs to be modified？

[refack]

@DuanPengfei the idea in not to change the behaviour of the old code, but add a new way to use it:

const path = require('path');
path.Pure.resolve('gaga') === path.Pure.posix.resolve('gaga') === path.Pure.windows.resolve('gaga') // either all throw or all returns just 'gaga'
// old code stays the same
path.posix.resolve('gaga') === 'C:\\bin\\dev\\node/gaga';

[DuanPengfei]

@refack I see

[DuanPengfei]

I will create a new PR do the same thing as the PR #13738. And then we discuss whether it is reasonable?

[refack]

With just the improvements? Sounds good.

[GongT]

path.posix.isAbsolute('//?/X:/yyy.txt') === true 😘

https://docs.microsoft.com/windows/desktop/FileIO/naming-a-file#win32-file-namespaces

[tniessen]

@GongT That seems to be correct. You are using the POSIX API, why would it recognize Windows namespaces?

[GongT]

@tniessen ? is valid file name on posix system. so //?/xxx is valid and absolute, that's works as indent.

[jeremyong]

I keep coming across this type of issue on github and the wild. The assumption that on Windows, the path separation and parsing desired is not POSIX is a bad assumption. People that develop in the wild use things like bash for windows, mingw, msys, etc. The API constraining you based on the OS used makes path among the least portable modules in NodeJS for developing cross platform scripts.

[jasnell]

This is actually a duplicate of #13683 ... closing this one