-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
fs: allow WHATWG URL and file: URLs as paths #10739
Conversation
I think the url label can be removed here? (Also, can you take a look at nodejs/github-bot#115 so that the bot will stop labeling these PRs with |
heh, yeah, the bot seems to be a bit aggressive about labeling things these days |
New CI: https://ci.nodejs.org/job/node-test-pull-request/5811/ |
lib/fs.js
Outdated
@@ -211,6 +213,7 @@ fs.access = function(path, mode, callback) { | |||
throw new TypeError('"callback" argument must be a function'); | |||
} | |||
|
|||
path = getPathFromURL(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WHATWG URL parsing could escape the null byte if it's not in the hostname ('\u0000' -> '%00', e.g. new URL('file://hostname/a/b/c\u0000')
), thus bypassing the null check, so might worth add a test for it. Is it possible for getPathFromURL
to put null bytes in a path without one? If not, maybe the nullCheck
can be performed before getPathFromURL
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. it's not just about null bytes, the path could contain any number of pct-encoded characters. This should run the path through a decode before returning... will fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that can be a problem for files with non-ASCII names...might worth add a test for something like 'file://hostname/文件.txt'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the hostname can be escaped by domain to ASCII too. Like 'file://你好你好/a/b'
lib/internal/url.js
Outdated
function isFileUrl(path) { | ||
if (typeof path !== 'string') return false; | ||
if (path.length < 7) return false; | ||
if ((path[0].codePointAt(0) | 0x20) !== 102) return false; // f F |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not path.codePointAt(0)
? Or path.startsWith('file://')
?
lib/internal/url.js
Outdated
} else { | ||
return path; | ||
} | ||
if (url.protocol !== 'file:') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this can be moved into the else if (path instanceof URL)
block, because a path that starts with file://
(isFileUrl
) will have the file protocol.
sigh... lol... obvious that last run was less than successful ;-) ... will investigate later on tonight |
Notable changes: * deps: * update V8 to 5.5 (Michaël Zasso) [#11029](nodejs/node#11029) * upgrade libuv to 1.11.0 (cjihrig) [#11094](nodejs/node#11094) * add node-inspect 1.10.4 (Jan Krems) [#10187](nodejs/node#10187) * upgrade zlib to 1.2.11 (Sam Roberts) [#10980](nodejs/node#10980) * lib: build `node inspect` into `node` (Anna Henningsen) [#10187](nodejs/node#10187) * crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [#9469](nodejs/node#9469) * inspector: add --inspect-brk (Josh Gavant) [#11149](nodejs/node#11149) * fs: allow WHATWG URL objects as paths (James M Snell) [#10739](nodejs/node#10739) * src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [#11129](nodejs/node#11129) * url: extend url.format to support WHATWG URL (James M Snell) [#10857](nodejs/node#10857) PR-URL: nodejs/node#11185 Signed-off-by: Ilkka Myller <[email protected]>
@addaleax said:
but I didn't see any follow up comments. Why is this not documented? I just found it while browsing the fs source. |
@sam-github Quoting the PR description (I think this had been added later):
|
I see. We can't document that fs APIs take a url.URL object when url.URL itself is undocumented. Fair enough. |
To be fair, |
Update fs module documentation adding WHATWG file URLS support for relevant fs functions/classes. Fixes: nodejs#12341 Refs: nodejs#10739
Update fs module documentation adding WHATWG file URLS support for relevant fs functions/classes. PR-URL: #12670 Fixes: #12341 Ref: #10739 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Update fs module documentation adding WHATWG file URLS support for relevant fs functions/classes. Fixes: nodejs#12341 Refs: nodejs#10739
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.
Updates the fs module APIs to allow
'file://' URL strings andWHATWG URL objects using 'file:' protocol to be passed as the path.For example:
On Windows, file: URLs with a hostname convert to UNC paths, while file: URLs with drive letters convert to local absolute paths:
On all other platforms, file: URLs with a hostname are unsupported and will result in a throw:
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: #10703
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
fs, url