Skip to content

Commit

Permalink
module: ensure 'node:'-only modules can access node_modules
Browse files Browse the repository at this point in the history
This commit allows require() and import to search the
node_modules directories when importing a core module that
must have the node: scheme. This prevents these core modules
from shadowing userland modules with the same name but no
prefix.

PR-URL: nodejs/node#42430
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
  • Loading branch information
cjihrig authored and guangwong committed Oct 10, 2022
1 parent 1b4a7f9 commit a63558e
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 18 deletions.
16 changes: 8 additions & 8 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,8 @@ if (isWindows) {
}

Module._resolveLookupPaths = function(request, parent) {
if (NativeModule.canBeRequiredByUsers(request)) {
if (NativeModule.canBeRequiredByUsers(request) &&
NativeModule.canBeRequiredWithoutScheme(request)) {
debug('looking for %j in []', request);
return null;
}
Expand Down Expand Up @@ -806,11 +807,8 @@ Module._load = function(request, parent, isMain) {
}

const mod = loadNativeModule(filename, request);
if (mod?.canBeRequiredByUsers) {
if (!NativeModule.canBeRequiredWithoutScheme(filename)) {
throw new ERR_UNKNOWN_BUILTIN_MODULE(filename);
}

if (mod?.canBeRequiredByUsers &&
NativeModule.canBeRequiredWithoutScheme(filename)) {
return mod.exports;
}

Expand Down Expand Up @@ -857,7 +855,8 @@ Module._load = function(request, parent, isMain) {

Module._resolveFilename = function(request, parent, isMain, options) {
if (StringPrototypeStartsWith(request, 'node:') ||
NativeModule.canBeRequiredByUsers(request)) {
(NativeModule.canBeRequiredByUsers(request) &&
NativeModule.canBeRequiredWithoutScheme(request))) {
return request;
}

Expand Down Expand Up @@ -1293,7 +1292,8 @@ Module._preloadModules = function(requests) {

Module.syncBuiltinESMExports = function syncBuiltinESMExports() {
for (const mod of NativeModule.map.values()) {
if (mod.canBeRequiredByUsers) {
if (mod.canBeRequiredByUsers &&
NativeModule.canBeRequiredWithoutScheme(mod.id)) {
mod.syncExports();
}
}
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,8 @@ class ESMLoader {
globalThis,
// Param getBuiltin
(builtinName) => {
if (NativeModule.canBeRequiredByUsers(builtinName)) {
if (NativeModule.canBeRequiredByUsers(builtinName) &&
NativeModule.canBeRequiredWithoutScheme(builtinName)) {
return require(builtinName);
}
throw new ERR_INVALID_ARG_VALUE('builtinName', builtinName);
Expand Down
11 changes: 4 additions & 7 deletions lib/internal/modules/esm/resolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ const {
ERR_PACKAGE_PATH_NOT_EXPORTED,
ERR_UNSUPPORTED_DIR_IMPORT,
ERR_NETWORK_IMPORT_DISALLOWED,
ERR_UNKNOWN_BUILTIN_MODULE,
ERR_UNSUPPORTED_ESM_URL_SCHEME,
} = require('internal/errors').codes;
const { Module: CJSModule } = require('internal/modules/cjs/loader');
Expand Down Expand Up @@ -888,11 +887,8 @@ function parsePackageName(specifier, base) {
* @returns {URL}
*/
function packageResolve(specifier, base, conditions) {
if (NativeModule.canBeRequiredByUsers(specifier)) {
if (!NativeModule.canBeRequiredWithoutScheme(specifier)) {
throw new ERR_UNKNOWN_BUILTIN_MODULE(specifier);
}

if (NativeModule.canBeRequiredByUsers(specifier) &&
NativeModule.canBeRequiredWithoutScheme(specifier)) {
return new URL('node:' + specifier);
}

Expand Down Expand Up @@ -1076,7 +1072,8 @@ function checkIfDisallowedImport(specifier, parsed, parsedParentURL) {

return { url: parsed.href };
}
if (NativeModule.canBeRequiredByUsers(specifier)) {
if (NativeModule.canBeRequiredByUsers(specifier) &&
NativeModule.canBeRequiredWithoutScheme(specifier)) {
throw new ERR_NETWORK_IMPORT_DISALLOWED(
specifier,
parsedParentURL,
Expand Down
40 changes: 38 additions & 2 deletions test/parallel/test-runner-import-no-scheme.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,51 @@
'use strict';
const common = require('../common');
const tmpdir = require('../common/tmpdir');
const assert = require('assert');
const { spawnSync } = require('child_process');
const fs = require('fs');
const path = require('path');
const { createRequire } = require('module');

assert.throws(
() => require('test'),
common.expectsError({ code: 'ERR_UNKNOWN_BUILTIN_MODULE' }),
common.expectsError({ code: 'MODULE_NOT_FOUND' }),
);

(async () => {
await assert.rejects(
async () => import('test'),
common.expectsError({ code: 'ERR_UNKNOWN_BUILTIN_MODULE' }),
common.expectsError({ code: 'ERR_MODULE_NOT_FOUND' }),
);
})().then(common.mustCall());

assert.throws(
() => require.resolve('test'),
common.expectsError({ code: 'MODULE_NOT_FOUND' }),
);

// Verify that files in node_modules can be resolved.
tmpdir.refresh();

const packageRoot = path.join(tmpdir.path, 'node_modules', 'test');
const indexFile = path.join(packageRoot, 'index.js');

fs.mkdirSync(packageRoot, { recursive: true });
fs.writeFileSync(indexFile, 'module.exports = { marker: 1 };');

function test(argv) {
const child = spawnSync(process.execPath, argv, { cwd: tmpdir.path });
assert.strictEqual(child.status, 0);
assert.strictEqual(child.stdout.toString().trim(), '{ marker: 1 }');
}

test(['-e', 'console.log(require("test"))']);
test(['-e', 'import("test").then(m=>console.log(m.default))']);
test(['--input-type=module', '-e', 'import test from "test";console.log(test)']);
test(['--input-type=module', '-e', 'console.log((await import("test")).default)']);

{
const dummyFile = path.join(tmpdir.path, 'file.js');
const require = createRequire(dummyFile);
assert.strictEqual(require.resolve('test'), indexFile);
}

0 comments on commit a63558e

Please sign in to comment.