Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

module: add support for node:‑prefixed require(…) calls #37246

Merged
merged 2 commits into from Mar 19, 2021
Merged

module: add support for node:‑prefixed require(…) calls #37246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
069b5df
module: add support for `node:`‑prefixed `require(…)` calls
ExE-Boss Feb 6, 2021
95391fe
repl: add auto‑completion for `node:`‑prefixed `require(…)` calls
ExE-Boss Feb 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/api/esm.md
View file
Open in desktop
Expand Up @@ -204,6 +204,10 @@ import _ from 'data:application/json,"world!"';
added:
- v14.13.1
- v12.20.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/37246
description: Added `node:` import support to `require(...)`.
-->

`node:` URLs are supported as an alternative means to load Node.js builtin
Expand Down
26 changes: 24 additions & 2 deletions doc/api/modules.md
View file
Open in desktop
Expand Up @@ -280,6 +280,12 @@ irrespective of whether or not `./foo` and `./FOO` are the same file.
## Core modules

<!--type=misc-->
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/37246
description: Added `node:` import support to `require(...)`.
-->

Node.js has several modules compiled into the binary. These modules are
described in greater detail elsewhere in this documentation.
Expand All @@ -291,6 +297,11 @@ Core modules are always preferentially loaded if their identifier is
passed to `require()`. For instance, `require('http')` will always
return the built in HTTP module, even if there is a file by that name.

Core modules can also be identified using the `node:` prefix, in which case
it bypasses the `require` cache. For instance, `require('node:http')` will
always return the built in HTTP module, even if there is `require.cache` entry
by that name.
richardlau marked this conversation as resolved.
Show resolved Hide resolved

## Cycles

<!--type=misc-->
Expand Down Expand Up @@ -642,8 +653,19 @@ error.

Adding or replacing entries is also possible. This cache is checked before
native modules and if a name matching a native module is added to the cache,
no require call is
going to receive the native module anymore. Use with care!
only `node:`-prefixed require calls are going to receive the native module.
Use with care!

```js
const assert = require('assert');
const realFs = require('fs');

const fakeFs = {};
require.cache.fs = { exports: fakeFs };

assert.strictEqual(require('fs'), fakeFs);
assert.strictEqual(require('node:fs'), realFs);
```

#### `require.extensions`
<!-- YAML
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/modules/cjs/helpers.js
View file
Open in desktop
Expand Up @@ -34,8 +34,9 @@ const cjsConditions = new SafeSet(['require', 'node', ...userConditions]);

function loadNativeModule(filename, request) {
const mod = NativeModule.map.get(filename);
if (mod) {
if (mod?.canBeRequiredByUsers) {
debug('load native module %s', request);
// compileForPublicLoader() throws if mod.canBeRequiredByUsers is false:
mod.compileForPublicLoader();
return mod;
}
Expand Down
17 changes: 15 additions & 2 deletions lib/internal/modules/cjs/loader.js
View file
Open in desktop
Expand Up @@ -110,7 +110,8 @@ let hasLoadedAnyUserCJSModule = false;
const {
ERR_INVALID_ARG_VALUE,
ERR_INVALID_MODULE_SPECIFIER,
ERR_REQUIRE_ESM
ERR_REQUIRE_ESM,
ERR_UNKNOWN_BUILTIN_MODULE,
} = require('internal/errors').codes;
const { validateString } = require('internal/validators');
const pendingDeprecation = getOptionValue('--pending-deprecation');
Expand Down Expand Up @@ -770,6 +771,17 @@ Module._load = function(request, parent, isMain) {
}

const filename = Module._resolveFilename(request, parent, isMain);
if (StringPrototypeStartsWith(filename, 'node:')) {
// Slice 'node:' prefix
const id = StringPrototypeSlice(filename, 5);

const module = loadNativeModule(id, request);
if (!module?.canBeRequiredByUsers) {
throw new ERR_UNKNOWN_BUILTIN_MODULE(filename);
}

return module.exports;
}
Comment on lines +774 to +784
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the motivation for short-circuiting the cache here, something that has never been done previously for native modules?

I don't see why this scheme should be any different to any other in this regard.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On this topic, should we throw or defer to cache on unknown builtins?


const cachedModule = Module._cache[filename];
if (cachedModule !== undefined) {
Expand Down Expand Up @@ -841,7 +853,8 @@ Module._load = function(request, parent, isMain) {
};

Module._resolveFilename = function(request, parent, isMain, options) {
if (NativeModule.canBeRequiredByUsers(request)) {
if (StringPrototypeStartsWith(request, 'node:') ||
NativeModule.canBeRequiredByUsers(request)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bifurcation will have implications for mocking, but I can appreciate the benefit too.

I'd still prefer a more convergent path here eg, to return node:fs for both node:fs and fs inputs, and then to still populate a cache['fs'] entry in the node:fs case as the same object for backwards compatibility. Such a change would hopefully be mostly compatible but would probably need to be a separate major nontheless. Worth thinking about at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing mocking tools (such as Jest) provide their own require(…) implementation, since they need to be able to bypass the mocked module using jest.requireActual(…).

return request;
}

Expand Down
2 changes: 1 addition & 1 deletion lib/internal/modules/esm/translators.js
View file
Open in desktop
Expand Up @@ -282,7 +282,7 @@ translators.set('builtin', async function builtinStrategy(url) {
debug(`Translating BuiltinModule ${url}`);
// Slice 'node:' scheme
const id = StringPrototypeSlice(url, 5);
const module = loadNativeModule(id, url, true);
const module = loadNativeModule(id, url);
if (!StringPrototypeStartsWith(url, 'node:') || !module) {
throw new ERR_UNKNOWN_BUILTIN_MODULE(url);
}
Expand Down
4 changes: 2 additions & 2 deletions lib/repl.js
View file
Open in desktop
Expand Up @@ -1113,7 +1113,7 @@ REPLServer.prototype.setPrompt = function setPrompt(prompt) {
};

const importRE = /\bimport\s*\(\s*['"`](([\w@./:-]+\/)?(?:[\w@./:-]*))(?![^'"`])$/;
const requireRE = /\brequire\s*\(\s*['"`](([\w@./-]+\/)?(?:[\w@./-]*))(?![^'"`])$/;
const requireRE = /\brequire\s*\(\s*['"`](([\w@./:-]+\/)?(?:[\w@./:-]*))(?![^'"`])$/;
const fsAutoCompleteRE = /fs(?:\.promises)?\.\s*[a-z][a-zA-Z]+\(\s*["'](.*)/;
const simpleExpressionRE =
/(?:[a-zA-Z_$](?:\w|\$)*\??\.)*[a-zA-Z_$](?:\w|\$)*\??\.?$/;
Expand Down Expand Up @@ -1275,7 +1275,7 @@ function complete(line, callback) {
}

if (!subdir) {
ArrayPrototypePush(completionGroups, _builtinLibs);
ArrayPrototypePush(completionGroups, _builtinLibs, nodeSchemeBuiltinLibs);
}
} else if (RegExpPrototypeTest(importRE, line) &&
this.allowBlockingCompletions) {
Expand Down
2 changes: 2 additions & 0 deletions test/es-module/test-esm-dynamic-import.js
View file
Open in desktop
Expand Up @@ -51,6 +51,8 @@ function expectFsNamespace(result) {

expectModuleError(import('node:unknown'),
'ERR_UNKNOWN_BUILTIN_MODULE');
expectModuleError(import('node:internal/test/binding'),
'ERR_UNKNOWN_BUILTIN_MODULE');
expectModuleError(import('./not-an-existing-module.mjs'),
'ERR_MODULE_NOT_FOUND');
expectModuleError(import('http://example.com/foo.js'),
Expand Down
18 changes: 13 additions & 5 deletions test/parallel/test-repl-tab-complete.js
View file
Open in desktop
Expand Up @@ -31,6 +31,10 @@ const assert = require('assert');
const path = require('path');
const fixtures = require('../common/fixtures');
const { builtinModules } = require('module');
const publicModules = builtinModules.filter(
(lib) => !lib.startsWith('_') && !lib.includes('/'),
);

const hasInspector = process.features.inspector;

if (!common.isMainThread)
Expand Down Expand Up @@ -239,9 +243,9 @@ putIn.run(['.clear']);

testMe.complete('require(\'', common.mustCall(function(error, data) {
assert.strictEqual(error, null);
builtinModules.forEach((lib) => {
publicModules.forEach((lib) => {
assert(
data[0].includes(lib) || lib.startsWith('_') || lib.includes('/'),
data[0].includes(lib) && data[0].includes(`node:${lib}`),
`${lib} not found`
);
});
Expand All @@ -258,11 +262,15 @@ testMe.complete("require\t( 'n", common.mustCall(function(error, data) {
assert.strictEqual(error, null);
assert.strictEqual(data.length, 2);
assert.strictEqual(data[1], 'n');
// require(...) completions include `node:`-prefixed modules:
publicModules.forEach((lib, index) =>
assert.strictEqual(data[0][index], `node:${lib}`));
assert.strictEqual(data[0][publicModules.length], '');
// There is only one Node.js module that starts with n:
assert.strictEqual(data[0][0], 'net');
assert.strictEqual(data[0][1], '');
assert.strictEqual(data[0][publicModules.length + 1], 'net');
assert.strictEqual(data[0][publicModules.length + 2], '');
// It's possible to pick up non-core modules too
data[0].slice(2).forEach((completion) => {
data[0].slice(publicModules.length + 3).forEach((completion) => {
assert.match(completion, /^n/);
});
}));
Expand Down
42 changes: 42 additions & 0 deletions test/parallel/test-require-node-prefix.js
View file
Open in desktop
@@ -0,0 +1,42 @@
'use strict';

require('../common');
const assert = require('assert');
const fs = require('fs');

const errUnknownBuiltinModuleRE = /^No such built-in module: /u;

// For direct use of require expressions inside of CJS modules,
// all kinds of specifiers should work without issue.
{
assert.strictEqual(require('fs'), fs);
assert.strictEqual(require('node:fs'), fs);

assert.throws(
() => require('node:unknown'),
{
code: 'ERR_UNKNOWN_BUILTIN_MODULE',
message: errUnknownBuiltinModuleRE,
},
);

assert.throws(
() => require('node:internal/test/binding'),
{
code: 'ERR_UNKNOWN_BUILTIN_MODULE',
message: errUnknownBuiltinModuleRE,
},
);
}

// `node:`-prefixed `require(...)` calls bypass the require cache:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this differs from other builtins, I love it, but want to be sure it is intentional.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this actually seems like an issue - it means someone who replaces a builtin intentionally can’t replace one of these (altho presumably mutating a builtin would be visible in both). That could cause an issue where someone wants to use a package to instrument or lock down fs (and thus, policies are not an ergonomic option, unless I’m misunderstanding how policies work), but accidentally leaves a gaping security hole when an attacker requires node:fs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

policies should intercept before any of the resolution helpers even get called

node/lib/internal/modules/cjs/helpers.js

Line 52 in 36cc0ee

if (redirects) {
, if any module redirection is in place (including complete shutdown) then it would never call out to module.require and get here without some kind of opt-in to the behavior (like dependencies:true).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least this needs documentation and probably a strong indication people using policies + instrumenting fs now need to update the policy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point was that policies aren’t currently required to ensure this for CJS - a package (not the app itself) can do it. Forcing policies to be the mechanism means packages are no longer capable of abstracting this for users.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally understood. Given that policies are the superior mechanism for app developers to lock things down, what's the benefit of adding special, inconsistent, cache-bypassing behavior for require with a node: prefix?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb i have no strong opinion on if we should diverge, but cache first behavior of CJS isn't shared by ESM and isn't robust against things like core destructuring the original impls. My like is just that it isn't a confusing situation like with fs/.

Copy link
Contributor Author

@ExE-Boss ExE-Boss Feb 8, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but cache first behavior of CJS isn't shared by ESM and isn't robust against things like core destructuring the original impls.

This is demonstrated by:

const fs = require('fs');

const fakeModule = {};
require.cache.fs = { exports: fakeModule };

assert.strictEqual((await import('fs')).default, fs);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, but that doesn’t mean it’s a good idea to make CJS inconsistent with itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think CJS is able to be claimed as consistent with itself since in general the natives don't normally populate the cache. I think it was just ad-hoc written together and the cache behavior is evolutionary not intentional or well understood (even by me).

{
const fakeModule = {};

require.cache.fs = { exports: fakeModule };

assert.strictEqual(require('fs'), fakeModule);
assert.strictEqual(require('node:fs'), fs);

delete require.cache.fs;
}