module: add support for node:‑prefixed require(…) calls#37246
Conversation
8615225 to
4f61960
Compare
|
You read my mind, I was precisely working on this. I noticed it produces an arguably better error when trying to load internal modules: $ node -p "require('internal/test/binding')"
Error: Cannot find module 'internal/test/binding'
$ node -p "require('node:internal/test/binding')"
Error: Should not compile internal/test/binding for public useCan you add tests in |
e740b61 to
320ef5e
Compare
|
cc @nodejs/modules |
| ); | ||
| } | ||
|
|
||
| // `node:`-prefixed `require(...)` calls bypass the require cache: |
There was a problem hiding this comment.
this differs from other builtins, I love it, but want to be sure it is intentional.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
policies should intercept before any of the resolution helpers even get called
node/lib/internal/modules/cjs/helpers.js
Line 52 in 36cc0ee
module.require and get here without some kind of opt-in to the behavior (like dependencies:true).
There was a problem hiding this comment.
At the very least this needs documentation and probably a strong indication people using policies + instrumenting fs now need to update the policy.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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/.
There was a problem hiding this comment.
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);There was a problem hiding this comment.
indeed, but that doesn’t mean it’s a good idea to make CJS inconsistent with itself.
There was a problem hiding this comment.
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).
3634cd4 to
d196d45
Compare
| Module._resolveFilename = function(request, parent, isMain, options) { | ||
| if (NativeModule.canBeRequiredByUsers(request)) { | ||
| if (StringPrototypeStartsWith(request, 'node:') || | ||
| NativeModule.canBeRequiredByUsers(request)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(…).
10b08bf to
8564814
Compare
| 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; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
On this topic, should we throw or defer to cache on unknown builtins?
8564814 to
b0fbc91
Compare
benjamingr
left a comment
benjamingr
left a comment
There was a problem hiding this comment.
Just making sure #37246 (comment) is addressed before landing :) (namely the caveat) - I'm appreciative of the work put in, thanks!
Also would like more @nodejs/modules and @nodejs/tsc eyes on this.
(The actual changes LGTM)
17 participants
Fixes: #36098 (applies only to commit 1d8c8b7)
Refs: #37178 (applies only to commit b0fbc91)