Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent results for path.toNamespacedPath() for win32 UNC paths #30224

Closed
btsimonh opened this issue Nov 2, 2019 · 2 comments · Fixed by #52915
Closed

Inconsistent results for path.toNamespacedPath() for win32 UNC paths #30224

btsimonh opened this issue Nov 2, 2019 · 2 comments · Fixed by #52915
Labels
path Issues and PRs related to the path subsystem. windows Issues and PRs related to the Windows platform.

Comments

@btsimonh
Copy link

btsimonh commented Nov 2, 2019

  • Version: v12.10.0
  • Platform: windows 10 64 bit
  • Subsystem: path/fs

when using UNC paths, the function path.toNamespacedPath() gives different results from non UNC paths.

This is reproduced by running the code attached below.
Basically, most of the time in Node, you can freely use '/' and '', and most repos just use '/', for example this snippet:
fs.readdirSync(__dirname + '/grant').forEach(function(filename) {
from
https://github.com/jaredhanson/oauth2orize/blob/master/lib/index.js

The issue is that if the path being accessed is a UNC style (actually, problem is only if accessed pre-namespaced?) path, then the '/' does not get translated to a '' on windows. If the path is a 'normal' drive based windows path, then the '/' is replaced by '' in path.toNamespacedPath(), which is used by most fs functions to qualify a path, and so there is no issue.

As you can see from the results of the simple tests, path.win32.resolve('\\?\c:\Windows/System') returns '\\?\c:\Windows\System'.
path.toNamespacedPath(path) uses this internally, but returns path, not resolvedPath in this circumstance. So the resulting path from path.toNamespacedPath('\\?\c:\Windows/System') is '\\?\c:\Windows/System' - which is invalid.
(I will post a proposed solution in the next comment).

Simple example test:

on a windows system, run the following code:

var tests = [
  'c:\\Windows\\System',
  'c:\\Windows/System',
  '\\\\?\\c:\\Windows\\System',
  '\\\\?\\c:\\Windows/System',
]

var path = require('path');
var fs = require('fs');

for (var i = 0; i < tests.length; i++) {
  console.log('path.toNamespacedPath("'+tests[i]+'") -> "'+path.toNamespacedPath(tests[i])+'"');
  console.log('path.win32.normalize("'+tests[i]+'") -> "'+path.win32.normalize(tests[i])+'"');
  console.log('path.win32.resolve("'+tests[i]+'") -> "'+path.win32.resolve(tests[i])+'"');
  try{
    var dir = fs.readdirSync(tests[i]);
    console.log('fs.readdirSync("'+tests[i]+'") success');
  } catch(e) {
    console.log(e);
    console.log('fs.readdirSync("'+tests[i]+'") failed');
  }
}
@btsimonh
Copy link
Author

btsimonh commented Nov 2, 2019

Suggest, in path.js:

  toNamespacedPath(path) {
    // Note: this will *probably* throw somewhere.
    if (typeof path !== 'string')
      return path;

    if (path.length === 0) {
      return '';
    }

    const resolvedPath = win32.resolve(path);

    if (resolvedPath.length <= 2)
      return path;

    if (resolvedPath.charCodeAt(0) === CHAR_BACKWARD_SLASH) {
      // Possible UNC root
      if (resolvedPath.charCodeAt(1) === CHAR_BACKWARD_SLASH) {
        const code = resolvedPath.charCodeAt(2);
        if (code !== CHAR_QUESTION_MARK && code !== CHAR_DOT) {
          // Matched non-long UNC root, convert the path to a long UNC path
          return `\\\\?\\UNC\\${resolvedPath.slice(2)}`;
        }
      }
    } else if (isWindowsDeviceRoot(resolvedPath.charCodeAt(0)) &&
               resolvedPath.charCodeAt(1) === CHAR_COLON &&
               resolvedPath.charCodeAt(2) === CHAR_BACKWARD_SLASH) {
      // Matched device root, convert the path to a long UNC path
      return `\\\\?\\${resolvedPath}`;
    }

    ////// Modified - since this is ONLY on windows, why not the resolved path? /////////////
    ////// is it not always our best result? /////////////
    return resolvedPath;
    ///////////////////
  },

@btsimonh
Copy link
Author

btsimonh commented Nov 2, 2019

p.s.

var tests = [
  'c:\\Windows\\System',
  'c:\\Windows/System',
  '\\\\localhost\\c$\\Windows\\System',
  '\\\\localhost\\c$\\Windows/System',
]

works fine.... so UNC paths are all good.

@targos targos added path Issues and PRs related to the path subsystem. windows Issues and PRs related to the Windows platform. labels Dec 26, 2020
nodejs-github-bot pushed a commit that referenced this issue May 13, 2024
PR-URL: #52915
Fixes: #30224
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this issue May 15, 2024
PR-URL: #52915
Fixes: #30224
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jun 17, 2024
PR-URL: #52915
Fixes: #30224
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jun 17, 2024
PR-URL: #52915
Fixes: #30224
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jun 17, 2024
PR-URL: #52915
Fixes: #30224
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this issue Jun 20, 2024
PR-URL: nodejs#52915
Fixes: nodejs#30224
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
PR-URL: nodejs#52915
Fixes: nodejs#30224
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
path Issues and PRs related to the path subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants