From 192a782221c84fbd63c451bd7029e7c9c9d5a95b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 29 Jan 2022 22:05:36 +0100 Subject: [PATCH 1/5] src: return proper URLs from node_api_get_module_file_name Using `file://${path}` does not properly escape special URL characters. --- src/node_api.cc | 5 ++-- test/node-api/test_general/test.js | 37 ++++++++++++++++++++++++------ 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index b1b85073508672..86e03d771dd5bb 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..3fe43ea14bb8c2 100644 --- a/test/node-api/test_general/test.js +++ b/test/node-api/test_general/test.js @@ -1,15 +1,38 @@ 'use strict'; const common = require('../../common'); +const tmpdir = require('../../common/tmpdir'); +const fs = require('fs'); +const path = require('path'); 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( + decodeURIComponent(test_general.filename), + `file://${filename}`); +} + +{ + 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); + const reportedFilename = require(urlTestFile).filename; + assert.doesNotMatch(reportedFilename, /foo%\?#bar/); + assert.strictEqual( + decodeURIComponent(reportedFilename), + `file://${urlTestFile}`); +} + +{ + const [ major, minor, patch, release ] = test_general.testGetNodeVersion(); + assert.strictEqual(process.version.split('-')[0], + `v${major}.${minor}.${patch}`); + assert.strictEqual(release, process.release.name); +} From c88d685ba2318d9b33207d585d7894e6e592359e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 30 Jan 2022 13:15:02 +0100 Subject: [PATCH 2/5] fixup! src: return proper URLs from node_api_get_module_file_name --- test/node-api/test_general/test.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/test/node-api/test_general/test.js b/test/node-api/test_general/test.js index 3fe43ea14bb8c2..58bdea16282349 100644 --- a/test/node-api/test_general/test.js +++ b/test/node-api/test_general/test.js @@ -4,6 +4,7 @@ const common = require('../../common'); const tmpdir = require('../../common/tmpdir'); 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'); @@ -13,9 +14,7 @@ tmpdir.refresh(); { // TODO(gabrielschulhof): This test may need updating if/when the filename // becomes a full-fledged URL. - assert.strictEqual( - decodeURIComponent(test_general.filename), - `file://${filename}`); + assert.strictEqual(test_general.filename, url.pathToFileURL(filename).href); } { @@ -25,9 +24,7 @@ tmpdir.refresh(); fs.copyFileSync(filename, urlTestFile); const reportedFilename = require(urlTestFile).filename; assert.doesNotMatch(reportedFilename, /foo%\?#bar/); - assert.strictEqual( - decodeURIComponent(reportedFilename), - `file://${urlTestFile}`); + assert.strictEqual(reportedFilename, url.pathToFileURL(urlTestFile).href); } { From c7880d0e861a342c7e17afac98c8dc81eef0dabc Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 30 Jan 2022 15:44:29 +0100 Subject: [PATCH 3/5] fixup! src: return proper URLs from node_api_get_module_file_name --- test/node-api/test_general/test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/node-api/test_general/test.js b/test/node-api/test_general/test.js index 58bdea16282349..2909f9b65ce691 100644 --- a/test/node-api/test_general/test.js +++ b/test/node-api/test_general/test.js @@ -18,12 +18,12 @@ tmpdir.refresh(); } { - const urlTestDir = path.join(tmpdir.path, 'foo%?#bar'); + 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); const reportedFilename = require(urlTestFile).filename; - assert.doesNotMatch(reportedFilename, /foo%\?#bar/); + assert.doesNotMatch(reportedFilename, /foo%#bar/); assert.strictEqual(reportedFilename, url.pathToFileURL(urlTestFile).href); } From a89ef435eebdeee91bfcb6a8d9f8ba450d25b5fd Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 15 Feb 2022 13:16:36 +0100 Subject: [PATCH 4/5] fixup! src: return proper URLs from node_api_get_module_file_name --- test/node-api/test_general/test.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/node-api/test_general/test.js b/test/node-api/test_general/test.js index 2909f9b65ce691..449045775681dd 100644 --- a/test/node-api/test_general/test.js +++ b/test/node-api/test_general/test.js @@ -25,6 +25,11 @@ tmpdir.refresh(); const reportedFilename = require(urlTestFile).filename; assert.doesNotMatch(reportedFilename, /foo%#bar/); assert.strictEqual(reportedFilename, url.pathToFileURL(urlTestFile).href); + fs.rmSync(urlTestDir, { + force: true, + recursive: true, + maxRetries: 256 + }); } { From b928626277882c3ca759789f348a5873bb61d5aa Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 21 Feb 2022 11:27:51 +0100 Subject: [PATCH 5/5] fixup! fixup! src: return proper URLs from node_api_get_module_file_name --- test/node-api/test_general/test.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/node-api/test_general/test.js b/test/node-api/test_general/test.js index 449045775681dd..77550c02927fb4 100644 --- a/test/node-api/test_general/test.js +++ b/test/node-api/test_general/test.js @@ -2,6 +2,7 @@ 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'); @@ -22,7 +23,12 @@ tmpdir.refresh(); const urlTestFile = path.join(urlTestDir, path.basename(filename)); fs.mkdirSync(urlTestDir, { recursive: true }); fs.copyFileSync(filename, urlTestFile); - const reportedFilename = require(urlTestFile).filename; + // 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, {