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.

Sign up for GitHub

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

Jump to bottom

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 •
edited

  • 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.

@btsimonh btsimonh mentioned this issue Nov 5, 2019
Open
5 tasks
@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
@huseyinacacak-janea huseyinacacak-janea mentioned this issue May 9, 2024
Merged
nodejs-github-bot pushed a commit that referenced this issue May 13, 2024
@huseyinacacak-janea
path: fix toNamespacedPath on Windows
dcd8b82 
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
@huseyinacacak-janea @targos
path: fix toNamespacedPath on Windows
609d90b 
PR-URL: #52915
Fixes: #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
Assignees
No one assigned
Labels
path Issues and PRs related to the path subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

path: fix toNamespacedPath on Windows JaneaSystems/node
2 participants
@targos @btsimonh