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

pathToFileURL function in url fails to handle special characters properly #54515

Closed
EarlyRiser42 opened this issue Aug 23, 2024 · 3 comments · Fixed by #54545
Closed

pathToFileURL function in url fails to handle special characters properly #54515

EarlyRiser42 opened this issue Aug 23, 2024 · 3 comments · Fixed by #54545

Comments

@EarlyRiser42
Copy link
Contributor

EarlyRiser42 commented Aug 23, 2024

Version

22.5.1

Platform

Linux earlyriser-virtual-machine 6.5.0-44-generic #44~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Tue Jun 18 14:36:16 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

$ node
Welcome to Node.js v22.5.1.
Type ".help" for more information.
> require('url').pathToFileURL('\\server\foobar[', { windows: true }).href;
'file:///C:/server%0Coobar['
> require('url').pathToFileURL('\\server\foobar[', { windows: false }).href;
'file:///Users/admin/%5Cserver%0Coobar['
> require('url').pathToFileURL('C:/path^', { windows: true }).href;
'file:///C:/path^'
> require('url').pathToFileURL('home/path^', { windows: false }).href;
'file:///Users/admin/home/path^'

How often does it reproduce? Is there a required condition?

everytime

What is the expected behavior? Why is that the expected behavior?

I think the expected behavior is:

> require('url').pathToFileURL('\\server\foobar[', { windows: true }).href;
'file:///C:/server%0Coobar%5E'
> require('url').pathToFileURL('\\server\foobar[', { windows: false }).href;
'file:///Users/admin/%5Cserver%0Coobar%5B'
> require('url').pathToFileURL('C:/path^', { windows: true }).href;
'file:///C:/path%5E'
> require('url').pathToFileURL('home/path^', { windows: false }).href;
'file:///Users/admin/home/path%5D'

because [ and ] are special characters used to specify IP addresses in URLs, and ^ is generally unsafe in URLs and should also be percent-encoded.

What do you see instead?

I see that [, ], and ^ are not encoded

Additional information

I think that setter and constructor both didn't encoded those special characters

@aduh95
Copy link
Contributor

aduh95 commented Aug 23, 2024

I'm not sure any of the examples you show are really a problem, IIUC those characters would be problematic only in the origin; in the path it's perfectly fine.

However, a problematic one would be:

$ node -p 'url.pathToFileURL("\\\\server[\\foobar[", { windows: true }).href'
file:///foobar[

@avivkeller
Copy link
Member

FWIW According to RFC1738, they should be encoded (see the bold):

Characters can be unsafe for a number of reasons. The space
character is unsafe
because significant spaces may disappear and
insignificant spaces may be introduced when URLs are transcribed or
typeset or subjected to the treatment of word-processing programs.
The characters "<" and ">" are unsafe because they are used as the
delimiters around URLs in free text; the quote mark (""") is used to
delimit URLs in some systems. The character "#" is unsafe and should
always be encoded because it is used in World Wide Web and in other
systems to delimit a URL from a fragment/anchor identifier that might
follow it. The character "%" is unsafe because it is used for
encodings of other characters. Other characters are unsafe because
gateways and other transport agents are known to sometimes modify
such characters. These characters are "{", "}", "|", "", "^", "~",
"[", "]", and "`".

All unsafe characters must always be encoded within a URL. For
example, the character "#" must be encoded within URLs even in
systems that do not normally deal with fragment or anchor
identifiers, so that if the URL is copied into another system that
does use them, it will not be necessary to change the URL encoding.

@EarlyRiser42
Copy link
Contributor Author

EarlyRiser42 commented Aug 24, 2024

When trying to access files using a file URL on Windows 11, such as C:Users\admin\Downloads\[path]\test[1].txt, I had to use the encoded version file:///C:/Users/admin/Downloads/%5Bpath%5D/test%5B1%5D.txt instead of file:///C:/Users/admin/Downloads/[path]/test[1].txt.

The same behavior occurred on Ubuntu 22.04 when accessing a file located at home/earlyriser/Downloads/[path]/test[1].txt. I had to use the URL file:///home/earlyriser/Downloads/%5Bpath%5D/test%5B1%5D.txt.

This behavior was also consistent with UNC paths. However, my PR failed four tests because these tests assume that the [ and ] characters do not need to be encoded in file URLs.

aduh95 added a commit to aduh95/node that referenced this issue Oct 18, 2024
Co-authored-by: EarlyRiser42 <tkfydtls464@gmail.com>
PR-URL: nodejs#54545
Fixes: nodejs#54515
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
aduh95 added a commit that referenced this issue Oct 19, 2024
Co-authored-by: EarlyRiser42 <tkfydtls464@gmail.com>
PR-URL: #54545
Fixes: #54515
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
Co-authored-by: EarlyRiser42 <tkfydtls464@gmail.com>
PR-URL: nodejs#54545
Fixes: nodejs#54515
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
tpoisseau pushed a commit to tpoisseau/node that referenced this issue Nov 21, 2024
Co-authored-by: EarlyRiser42 <tkfydtls464@gmail.com>
PR-URL: nodejs#54545
Fixes: nodejs#54515
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
aduh95 added a commit that referenced this issue Nov 27, 2024
Co-authored-by: EarlyRiser42 <tkfydtls464@gmail.com>
PR-URL: #54545
Fixes: #54515
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
ruyadorno pushed a commit that referenced this issue Nov 27, 2024
Co-authored-by: EarlyRiser42 <tkfydtls464@gmail.com>
PR-URL: #54545
Fixes: #54515
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants