src: return proper URLs from node_api_get_module_file_name

Using `file://${path}` does not properly escape special URL characters. PR-URL: #41758 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
nodejs · Mar 7, 2022 · 98b1be0 · 98b1be0
1 parent ecb5980
commit 98b1be0
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 9 deletions.
diff --git 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
@@ -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);
+}