Repository: nodejs/node
Pull request: url: handle "unsafe" characters properly in `pathToFileURL`

Fixes: https://github.com/nodejs/node/issues/54515

Given the number of characters to cover, I leaning towards using one regex to deal with them all rather than adding more special cases. Let's check what the benchmark says.

> FWIW According to [RFC1738](https://www.ietf.org/rfc/rfc1738.txt), 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.

_Originally posted by @RedYetiDev in https://github.com/nodejs/node/issues/54515#issuecomment-2307957099_

I took the tests from https://github.com/nodejs/node/pull/54516, so I added @EarlyRiser42 as co-author.
            

<!--
Before submitting a pull request, please read:

- the CONTRIBUTING guide at https://github.com/nodejs/node/blob/HEAD/CONTRIBUTING.md
- the commit message formatting guidelines at
  https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

For code changes:
1. Include tests for any bug fixes or new features.
2. Update documentation if relevant.
3. Ensure that `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes.

If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the `notable-change` label.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
-->


Changed files:
filename: lib/internal/process/execution.js
patch: @@ -49,7 +49,7 @@ function tryGetCwd() {
 
 let evalIndex = 0;
 function getEvalModuleUrl() {
-  return pathToFileURL(`${process.cwd()}/[eval${++evalIndex}]`).href;
+  return `${pathToFileURL(process.cwd())}/[eval${++evalIndex}]`;
 }
 
 /**

filename: lib/internal/url.js
patch: @@ -1498,44 +1498,75 @@ function fileURLToPath(path, options = kEmptyObject) {
   return (windows ?? isWindows) ? getPathFromURLWin32(path) : getPathFromURLPosix(path);
 }
 
-// The following characters are percent-encoded when converting from file path
-// to URL:
-// - %: The percent character is the only character not encoded by the
-//        `pathname` setter.
-// - \: Backslash is encoded on non-windows platforms since it's a valid
-//      character but the `pathname` setters replaces it by a forward slash.
-// - LF: The newline character is stripped out by the `pathname` setter.
-//       (See whatwg/url#419)
-// - CR: The carriage return character is also stripped out by the `pathname`
-//       setter.
-// - TAB: The tab character is also stripped out by the `pathname` setter.
+// RFC1738 defines the following chars as "unsafe" for URLs
+// @see https://www.ietf.org/rfc/rfc1738.txt 2.2. URL Character Encoding Issues
 const percentRegEx = /%/g;
-const backslashRegEx = /\\/g;
 const newlineRegEx = /\n/g;
 const carriageReturnRegEx = /\r/g;
 const tabRegEx = /\t/g;
-const questionRegex = /\?/g;
+const quoteRegEx = /"/g;
 const hashRegex = /#/g;
+const spaceRegEx = / /g;
+const questionMarkRegex = /\?/g;
+const openSquareBracketRegEx = /\[/g;
+const backslashRegEx = /\\/g;
+const closeSquareBracketRegEx = /]/g;
+const caretRegEx = /\^/g;
+const verticalBarRegEx = /\|/g;
+const tildeRegEx = /~/g;
 
 function encodePathChars(filepath, options = kEmptyObject) {
-  const windows = options?.windows;
-  if (StringPrototypeIndexOf(filepath, '%') !== -1)
+  if (StringPrototypeIncludes(filepath, '%')) {
     filepath = RegExpPrototypeSymbolReplace(percentRegEx, filepath, '%25');
-  // In posix, backslash is a valid character in paths:
-  if (!(windows ?? isWindows) && StringPrototypeIndexOf(filepath, '\\') !== -1)
-    filepath = RegExpPrototypeSymbolReplace(backslashRegEx, filepath, '%5C');
-  if (StringPrototypeIndexOf(filepath, '\n') !== -1)
+  }
+
+  if (StringPrototypeIncludes(filepath, '\t')) {
+    filepath = RegExpPrototypeSymbolReplace(tabRegEx, filepath, '%09');
+  }
+  if (StringPrototypeIncludes(filepath, '\n')) {
     filepath = RegExpPrototypeSymbolReplace(newlineRegEx, filepath, '%0A');
-  if (StringPrototypeIndexOf(filepath, '\r') !== -1)
+  }
+  if (StringPrototypeIncludes(filepath, '\r')) {
     filepath = RegExpPrototypeSymbolReplace(carriageReturnRegEx, filepath, '%0D');
-  if (StringPrototypeIndexOf(filepath, '\t') !== -1)
-    filepath = RegExpPrototypeSymbolReplace(tabRegEx, filepath, '%09');
+  }
+  if (StringPrototypeIncludes(filepath, ' ')) {
+    filepath = RegExpPrototypeSymbolReplace(spaceRegEx, filepath, '%20');
+  }
+  if (StringPrototypeIncludes(filepath, '"')) {
+    filepath = RegExpPrototypeSymbolReplace(quoteRegEx, filepath, '%22');
+  }
+  if (StringPrototypeIncludes(filepath, '#')) {
+    filepath = RegExpPrototypeSymbolReplace(hashRegex, filepath, '%23');
+  }
+  if (StringPrototypeIncludes(filepath, '?')) {
+    filepath = RegExpPrototypeSymbolReplace(questionMarkRegex, filepath, '%3F');
+  }
+  if (StringPrototypeIncludes(filepath, '[')) {
+    filepath = RegExpPrototypeSymbolReplace(openSquareBracketRegEx, filepath, '%5B');
+  }
+  // Back-slashes must be special-cased on Windows, where they are treated as path separator.
+  if (!options.windows && StringPrototypeIncludes(filepath, '\\')) {
+    filepath = RegExpPrototypeSymbolReplace(backslashRegEx, filepath, '%5C');
+  }
+  if (StringPrototypeIncludes(filepath, ']')) {
+    filepath = RegExpPrototypeSymbolReplace(closeSquareBracketRegEx, filepath, '%5D');
+  }
+  if (StringPrototypeIncludes(filepath, '^')) {
+    filepath = RegExpPrototypeSymbolReplace(caretRegEx, filepath, '%5E');
+  }
+  if (StringPrototypeIncludes(filepath, '|')) {
+    filepath = RegExpPrototypeSymbolReplace(verticalBarRegEx, filepath, '%7C');
+  }
+  if (StringPrototypeIncludes(filepath, '~')) {
+    filepath = RegExpPrototypeSymbolReplace(tildeRegEx, filepath, '%7E');
+  }
+
   return filepath;
 }
 
 function pathToFileURL(filepath, options = kEmptyObject) {
-  const windows = options?.windows;
-  if ((windows ?? isWindows) && StringPrototypeStartsWith(filepath, '\\\\')) {
+  const windows = options?.windows ?? isWindows;
+  if (windows && StringPrototypeStartsWith(filepath, '\\\\')) {
     const outURL = new URL('file://');
     // UNC path format: \\server\share\resource
     // Handle extended UNC path and standard UNC path
@@ -1566,20 +1597,9 @@ function pathToFileURL(filepath, options = kEmptyObject) {
     );
     return outURL;
   }
-  let resolved = (windows ?? isWindows) ? path.win32.resolve(filepath) : path.posix.resolve(filepath);
-
-  // Call encodePathChars first to avoid encoding % again for ? and #.
-  resolved = encodePathChars(resolved, { windows });
+  const resolved = windows ? path.win32.resolve(filepath) : path.posix.resolve(filepath);
 
-  // Question and hash character should be included in pathname.
-  // Therefore, encoding is required to eliminate parsing them in different states.
-  // This is done as an optimization to not creating a URL instance and
-  // later triggering pathname setter, which impacts performance
-  if (StringPrototypeIndexOf(resolved, '?') !== -1)
-    resolved = RegExpPrototypeSymbolReplace(questionRegex, resolved, '%3F');
-  if (StringPrototypeIndexOf(resolved, '#') !== -1)
-    resolved = RegExpPrototypeSymbolReplace(hashRegex, resolved, '%23');
-  return new URL(`file://${resolved}`);
+  return new URL(`file://${encodePathChars(resolved, { windows })}`);
 }
 
 function toPathIfFileURL(fileURLOrPath) {

filename: test/parallel/test-url-pathtofileurl.js
patch: @@ -13,10 +13,7 @@ const url = require('url');
 {
   const fileURL = url.pathToFileURL('test\\').href;
   assert.ok(fileURL.startsWith('file:///'));
-  if (isWindows)
-    assert.ok(fileURL.endsWith('/'));
-  else
-    assert.ok(fileURL.endsWith('%5C'));
+  assert.match(fileURL, isWindows ? /\/$/ : /%5C$/);
 }
 
 {
@@ -104,6 +101,12 @@ const windowsTestCases = [
   { path: 'C:\\€', expected: 'file:///C:/%E2%82%AC' },
   // Rocket emoji (non-BMP code point)
   { path: 'C:\\🚀', expected: 'file:///C:/%F0%9F%9A%80' },
+  // caret
+  { path: 'C:\\foo^bar', expected: 'file:///C:/foo%5Ebar' },
+  // left bracket
+  { path: 'C:\\foo[bar', expected: 'file:///C:/foo%5Bbar' },
+  // right bracket
+  { path: 'C:\\foo]bar', expected: 'file:///C:/foo%5Dbar' },
   // Local extended path
   { path: '\\\\?\\C:\\path\\to\\file.txt', expected: 'file:///C:/path/to/file.txt' },
   // UNC path (see https://docs.microsoft.com/en-us/archive/blogs/ie/file-uris-in-windows)
@@ -154,6 +157,8 @@ const posixTestCases = [
   { path: '/€', expected: 'file:///%E2%82%AC' },
   // Rocket emoji (non-BMP code point)
   { path: '/🚀', expected: 'file:///%F0%9F%9A%80' },
+  // "unsafe" chars
+  { path: '/foo\r\n\t<>"#%{}|^[\\~]`?bar', expected: 'file:///foo%0D%0A%09%3C%3E%22%23%25%7B%7D%7C%5E%5B%5C%7E%5D%60%3Fbar' },
 ];
 
 for (const { path, expected } of windowsTestCases) {