From 98b1be0ec2722abdda43c61d375cb8558db19203 Mon Sep 17 00:00:00 2001 From: Anna Henningsen 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 Reviewed-By: Tobias Nießen Reviewed-By: Juan José Arboleda Reviewed-By: Michael Dawson --- 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 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); +}