Skip to content

Commit

Permalink
src: return proper URLs from node_api_get_module_file_name
Browse files Browse the repository at this point in the history
Using `file://${path}` does not properly escape special URL characters.

PR-URL: #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>
  • Loading branch information
addaleax committed Mar 7, 2022
1 parent 4158d62 commit 64832c1
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 9 deletions.
5 changes: 3 additions & 2 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down
45 changes: 38 additions & 7 deletions test/node-api/test_general/test.js
Original file line number Diff line number Diff line change
@@ -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);
}

0 comments on commit 64832c1

Please sign in to comment.