From 62b74e5306fed531bc10d1b2fa192f5ff2433ea3 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sat, 29 Jan 2022 22:05:36 +0100
Subject: [PATCH] src: return proper URLs from node_api_get_module_file_name
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Using `file://${path}` does not properly escape special URL characters.

PR-URL: https://github.com/nodejs/node/pull/41758
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
---
 src/node_api.cc                    |  5 ++--
 test/node-api/test_general/test.js | 45 +++++++++++++++++++++++++-----
 2 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/src/node_api.cc b/src/node_api.cc
index 82e228bb954d5a..60fbe96b8ef272 100644
--- a/src/node_api.cc
+++ b/src/node_api.cc
@@ -9,6 +9,7 @@
 #include "node_buffer.h"
 #include "node_errors.h"
 #include "node_internals.h"
+#include "node_url.h"
 #include "threadpoolwork-inl.h"
 #include "tracing/traced_value.h"
 #include "util-inl.h"
@@ -589,13 +590,13 @@ void napi_module_register_by_symbol(v8::Local<v8::Object> exports,
   if (module->ToObject(context).ToLocal(&modobj) &&
       modobj->Get(context, node_env->filename_string()).ToLocal(&filename_js) &&
       filename_js->IsString()) {
-    node::Utf8Value filename(node_env->isolate(), filename_js);  // Cast
+    node::Utf8Value filename(node_env->isolate(), filename_js);
 
     // Turn the absolute path into a URL. Currently the absolute path is always
     // a file system path.
     // TODO(gabrielschulhof): Pass the `filename` through unchanged if/when we
     // receive it as a URL already.
-    module_filename = std::string("file://") + (*filename);
+    module_filename = node::url::URL::FromFilePath(filename.ToString()).href();
   }
 
   // Create a new napi_env for this specific module.
diff --git a/test/node-api/test_general/test.js b/test/node-api/test_general/test.js
index dd409f010a3ada..77550c02927fb4 100644
--- a/test/node-api/test_general/test.js
+++ b/test/node-api/test_general/test.js
@@ -1,15 +1,46 @@
 'use strict';
 
 const common = require('../../common');
+const tmpdir = require('../../common/tmpdir');
+const child_process = require('child_process');
+const fs = require('fs');
+const path = require('path');
+const url = require('url');
 const filename = require.resolve(`./build/${common.buildType}/test_general`);
 const test_general = require(filename);
 const assert = require('assert');
 
-// TODO(gabrielschulhof): This test may need updating if/when the filename
-// becomes a full-fledged URL.
-assert.strictEqual(test_general.filename, `file://${filename}`);
+tmpdir.refresh();
 
-const [ major, minor, patch, release ] = test_general.testGetNodeVersion();
-assert.strictEqual(process.version.split('-')[0],
-                   `v${major}.${minor}.${patch}`);
-assert.strictEqual(release, process.release.name);
+{
+  // TODO(gabrielschulhof): This test may need updating if/when the filename
+  // becomes a full-fledged URL.
+  assert.strictEqual(test_general.filename, url.pathToFileURL(filename).href);
+}
+
+{
+  const urlTestDir = path.join(tmpdir.path, 'foo%#bar');
+  const urlTestFile = path.join(urlTestDir, path.basename(filename));
+  fs.mkdirSync(urlTestDir, { recursive: true });
+  fs.copyFileSync(filename, urlTestFile);
+  // Use a child process as indirection so that the native module is not loaded
+  // into this process and can be removed here.
+  const reportedFilename = child_process.spawnSync(
+    process.execPath,
+    ['-p', `require(${JSON.stringify(urlTestFile)}).filename`],
+    { encoding: 'utf8' }).stdout.trim();
+  assert.doesNotMatch(reportedFilename, /foo%#bar/);
+  assert.strictEqual(reportedFilename, url.pathToFileURL(urlTestFile).href);
+  fs.rmSync(urlTestDir, {
+    force: true,
+    recursive: true,
+    maxRetries: 256
+  });
+}
+
+{
+  const [ major, minor, patch, release ] = test_general.testGetNodeVersion();
+  assert.strictEqual(process.version.split('-')[0],
+                     `v${major}.${minor}.${patch}`);
+  assert.strictEqual(release, process.release.name);
+}