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

not support file URLs containing drive specifiers (e.g., "file:///c:/path") #10703

Closed
mayyamus opened this issue Jan 9, 2017 · 8 comments
Closed
Assignees
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Comments

@mayyamus
Copy link

mayyamus commented Jan 9, 2017

  • Version:All
  • Platform:windows
  • Subsystem:fs

Remove leading slashes if followed by drive specifier.As a side effect,"/c:/path" can be used as an alternative to "c:/path".

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Jan 9, 2017
@bnoordhuis bnoordhuis added the feature request Issues that request new features to be added to Node.js. label Jan 10, 2017
@bnoordhuis
Copy link
Member

It's not entirely clear what you are requesting. Can you post some example code and explain how you would like it to work?

@bzoz
Copy link
Contributor

bzoz commented Jan 10, 2017

@mayyamus I'm also not sure what is the issue here. With node I get:

> fs.readFileSync('/c:/test')
Error: ENOENT: no such file or directory, open 'C:\c:\test'

@jasnell
Copy link
Member

jasnell commented Jan 10, 2017

@bnoordhuis @bzoz, if I understand correctly, the issue is that with file:// URLs that contain Windows drive letters, e.g. file:///C:/Test, the pathname component is output as /C:/Test. Conceivably, on Windows, the fs module could be modified to detect if the path used in fs methods follows this pattern and could strip the leading / off if the immediately following segment is a drive letter.

Alternatively, the fs module could be modified to accept file:// URL objects as the path, which would be fairly interesting on it's own.

const fs = require('fs');
const URL = require('url').URL;
const myFileURL = new URL('file:///C:/Test/foo.txt');
fs.readFile(myFileURL, (err, data) => { /**...**/ });

@jasnell jasnell added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 10, 2017
@jasnell jasnell self-assigned this Jan 10, 2017
@mayyamus
Copy link
Author

mayyamus commented Jan 11, 2017

@jasnell @bzoz @bnoordhuis Adding a method support URL is neccessary,so fs'constructors more perfectible.In final,you also should remove first slash('/') in normalize method.

@bnoordhuis
Copy link
Member

Conceivably, on Windows, the fs module could be modified to detect if the path used in fs methods follows this pattern and could strip the leading / off if the immediately following segment is a drive letter.

That seems too much like second-guessing the user. There are probably edge cases (UNC paths?) where it breaks down.

Alternatively, the fs module could be modified to accept file:// URL objects as the path, which would be fairly interesting on it's own.

That might be an acceptable change.

@jasnell
Copy link
Member

jasnell commented Jan 11, 2017

There are probably edge cases (UNC paths?) where it breaks down.

Using the new URL parser it should not specifically because the parsing algorithm handles Windows drive letters as a special case. What we can essentially do here is this:

  1. If path is a string prefixed with file://, we create a URL object
  2. Accept URL objects passed in as the path
  3. If protocol is anything other than file://, throw
  4. If hostname is set in the URL, then it is a UNC path, otherwise it's a local path
  5. If it's a local path, strip the leading /, otherwise construct the appropriate UNC path

@mayyamus
Copy link
Author

mayyamus commented Jan 11, 2017

var _url = require('url');
var urlInfo = (0,_url.parse)('file:///c:/path');
var path = urlInfo.path;
Now,path'value is '/c:/path'.so,all input path(type is string) must check:
0. path.length >=3
1.path.charCodeAt(0) is slash('/')
2.path.charCodeAt(1) is windows drive letters
3.path.charCodeAt(2) is ':'
if all passed,then is local file.so should strip the leading slash('/')

jasnell added a commit that referenced this issue Feb 3, 2017
Updates the fs module APIs to allow 'file://' URL objects
to be passed as the path.

For example:

```js
const URL = require('url').URL;
const myURL = new URL('file:///C:/path/to/file');
fs.readFile(myURL, (err, data) => {});
```

On Windows, file: URLs with a hostname convert to UNC paths,
while file: URLs with drive letters convert to local absolute
paths:

```
file://hostname/a/b/c => \\hostname\a\b\c
file:///c:/a/b/c => c:\a\b\c
```

On all other platforms, file: URLs with a hostname are unsupported
and will result in a throw:

```
file://hostname/a/b/c => throw!
file:///a/b/c => /a/b/c
```

The documentation for the fs API is intentionally not updated in
this commit because the URL API is still considered experimental
and is not officially documented *at this time*

Note that file: URLs are *required* by spec to always be absolute
paths from the file system root.

This is a semver-major commit because it changes error handling
on the fs APIs.

PR-URL: #10739
Ref: #10703
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jasnell added a commit to jasnell/node that referenced this issue Feb 6, 2017
Updates the fs module APIs to allow 'file://' URL objects
to be passed as the path.

For example:

```js
const URL = require('url').URL;
const myURL = new URL('file:///C:/path/to/file');
fs.readFile(myURL, (err, data) => {});
```

On Windows, file: URLs with a hostname convert to UNC paths,
while file: URLs with drive letters convert to local absolute
paths:

```
file://hostname/a/b/c => \\hostname\a\b\c
file:///c:/a/b/c => c:\a\b\c
```

On all other platforms, file: URLs with a hostname are unsupported
and will result in a throw:

```
file://hostname/a/b/c => throw!
file:///a/b/c => /a/b/c
```

The documentation for the fs API is intentionally not updated in
this commit because the URL API is still considered experimental
and is not officially documented *at this time*

Note that file: URLs are *required* by spec to always be absolute
paths from the file system root.

This is a semver-major commit because it changes error handling
on the fs APIs.

Refs: nodejs#10703
jasnell added a commit that referenced this issue Feb 6, 2017
Updates the fs module APIs to allow 'file://' URL objects
to be passed as the path.

For example:

```js
const URL = require('url').URL;
const myURL = new URL('file:///C:/path/to/file');
fs.readFile(myURL, (err, data) => {});
```

On Windows, file: URLs with a hostname convert to UNC paths,
while file: URLs with drive letters convert to local absolute
paths:

```
file://hostname/a/b/c => \\hostname\a\b\c
file:///c:/a/b/c => c:\a\b\c
```

On all other platforms, file: URLs with a hostname are unsupported
and will result in a throw:

```
file://hostname/a/b/c => throw!
file:///a/b/c => /a/b/c
```

The documentation for the fs API is intentionally not updated in
this commit because the URL API is still considered experimental
and is not officially documented *at this time*

Note that file: URLs are *required* by spec to always be absolute
paths from the file system root.

This is a semver-major commit because it changes error handling
on the fs APIs.

PR-URL: #10739
Ref: #10703
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas pushed a commit to italoacasas/node that referenced this issue Feb 13, 2017
Updates the fs module APIs to allow 'file://' URL objects
to be passed as the path.

For example:

```js
const URL = require('url').URL;
const myURL = new URL('file:///C:/path/to/file');
fs.readFile(myURL, (err, data) => {});
```

On Windows, file: URLs with a hostname convert to UNC paths,
while file: URLs with drive letters convert to local absolute
paths:

```
file://hostname/a/b/c => \\hostname\a\b\c
file:///c:/a/b/c => c:\a\b\c
```

On all other platforms, file: URLs with a hostname are unsupported
and will result in a throw:

```
file://hostname/a/b/c => throw!
file:///a/b/c => /a/b/c
```

The documentation for the fs API is intentionally not updated in
this commit because the URL API is still considered experimental
and is not officially documented *at this time*

Note that file: URLs are *required* by spec to always be absolute
paths from the file system root.

This is a semver-major commit because it changes error handling
on the fs APIs.

PR-URL: nodejs#10739
Ref: nodejs#10703
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas pushed a commit to italoacasas/node that referenced this issue Feb 14, 2017
Updates the fs module APIs to allow 'file://' URL objects
to be passed as the path.

For example:

```js
const URL = require('url').URL;
const myURL = new URL('file:///C:/path/to/file');
fs.readFile(myURL, (err, data) => {});
```

On Windows, file: URLs with a hostname convert to UNC paths,
while file: URLs with drive letters convert to local absolute
paths:

```
file://hostname/a/b/c => \\hostname\a\b\c
file:///c:/a/b/c => c:\a\b\c
```

On all other platforms, file: URLs with a hostname are unsupported
and will result in a throw:

```
file://hostname/a/b/c => throw!
file:///a/b/c => /a/b/c
```

The documentation for the fs API is intentionally not updated in
this commit because the URL API is still considered experimental
and is not officially documented *at this time*

Note that file: URLs are *required* by spec to always be absolute
paths from the file system root.

This is a semver-major commit because it changes error handling
on the fs APIs.

PR-URL: nodejs#10739
Ref: nodejs#10703
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
krydos pushed a commit to krydos/node that referenced this issue Feb 25, 2017
Updates the fs module APIs to allow 'file://' URL objects
to be passed as the path.

For example:

```js
const URL = require('url').URL;
const myURL = new URL('file:///C:/path/to/file');
fs.readFile(myURL, (err, data) => {});
```

On Windows, file: URLs with a hostname convert to UNC paths,
while file: URLs with drive letters convert to local absolute
paths:

```
file://hostname/a/b/c => \\hostname\a\b\c
file:///c:/a/b/c => c:\a\b\c
```

On all other platforms, file: URLs with a hostname are unsupported
and will result in a throw:

```
file://hostname/a/b/c => throw!
file:///a/b/c => /a/b/c
```

The documentation for the fs API is intentionally not updated in
this commit because the URL API is still considered experimental
and is not officially documented *at this time*

Note that file: URLs are *required* by spec to always be absolute
paths from the file system root.

This is a semver-major commit because it changes error handling
on the fs APIs.

PR-URL: nodejs#10739
Ref: nodejs#10703
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
krydos pushed a commit to krydos/node that referenced this issue Feb 25, 2017
Updates the fs module APIs to allow 'file://' URL objects
to be passed as the path.

For example:

```js
const URL = require('url').URL;
const myURL = new URL('file:///C:/path/to/file');
fs.readFile(myURL, (err, data) => {});
```

On Windows, file: URLs with a hostname convert to UNC paths,
while file: URLs with drive letters convert to local absolute
paths:

```
file://hostname/a/b/c => \\hostname\a\b\c
file:///c:/a/b/c => c:\a\b\c
```

On all other platforms, file: URLs with a hostname are unsupported
and will result in a throw:

```
file://hostname/a/b/c => throw!
file:///a/b/c => /a/b/c
```

The documentation for the fs API is intentionally not updated in
this commit because the URL API is still considered experimental
and is not officially documented *at this time*

Note that file: URLs are *required* by spec to always be absolute
paths from the file system root.

This is a semver-major commit because it changes error handling
on the fs APIs.

PR-URL: nodejs#10739
Ref: nodejs#10703
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@watilde
Copy link
Member

watilde commented Apr 22, 2017

This issue seems to be resolved by #10739. I'm going to close.

@watilde watilde closed this as completed Apr 22, 2017
dobriai pushed a commit to dobriai/electron-oauth2-3legged that referenced this issue Apr 21, 2018
Looks like there is some junk with Windows paths. To put it simply, the
following call is an Identity operation under Linux, but leaves a
leading slash under Windows:

url.parse( url.format( { pathname: path.join(process.cwd(), 'got_access_token.html'), protocol: 'elif:', slashes: true } )).pathname

On windows you get '/E:/some/path/...' - note the leading slash.

Some discussions here:
  nodejs/node#10703
  nodejs/node#10739

Dunno! I just fixed it by hand.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

No branches or pull requests

6 participants