From 884d2fddc4eff51e62536682bb1dd9ef7a81b18c Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Tue, 18 Mar 2025 23:45:05 +0000 Subject: [PATCH 1/6] module: remove underscore modules from builtinModules and isBuiltin these changes remove modules starting with underscore (e.g. `__http_agent`) from `Module#builtinModules` and `Module#isBuiltin` since they are not properly documented as public. They can't easily be removed since many libraries use them, but their usage should be discouraged and they should not be exposed by such utilities --- lib/internal/bootstrap/realm.js | 26 +++++++++++++++++++++----- lib/internal/modules/cjs/loader.js | 2 +- test/parallel/test-module-builtin.js | 7 +++++++ test/parallel/test-module-isBuiltin.js | 18 ++++++++++++++++++ 4 files changed, 47 insertions(+), 6 deletions(-) diff --git a/lib/internal/bootstrap/realm.js b/lib/internal/bootstrap/realm.js index f49f0814bbc687..79c7e234e7711d 100644 --- a/lib/internal/bootstrap/realm.js +++ b/lib/internal/bootstrap/realm.js @@ -310,9 +310,20 @@ class BuiltinModule { } static isBuiltin(id) { - return BuiltinModule.canBeRequiredWithoutScheme(id) || ( - typeof id === 'string' && - StringPrototypeStartsWith(id, 'node:') && + if (typeof id !== 'string') { + return false; + } + + if (StringPrototypeStartsWith(id, '_')) { + return false; + } + + if (BuiltinModule.canBeRequiredWithoutScheme(id)) { + return true; + } + + return ( + StringPrototypeStartsWith(id, 'node:') && BuiltinModule.canBeRequiredByUsers(StringPrototypeSlice(id, 5)) ); } @@ -321,10 +332,15 @@ class BuiltinModule { return ArrayFrom(schemelessBlockList); } - static getAllBuiltinModuleIds() { + static getAllPublicBuiltinModuleIds() { const allBuiltins = ArrayFrom(canBeRequiredByUsersWithoutSchemeList); ArrayPrototypePushApply(allBuiltins, ArrayFrom(schemelessBlockList, (x) => `node:${x}`)); - return allBuiltins; + return ArrayPrototypeFilter(allBuiltins, (id) => { + // Modules starting with an underscore (e.g. `_http_agent`) can be + // required by users because of historical reasons, but they are not + // proper documented public modules, so they need to be filtered out + return id[0] !== '_'; + }); } // Used by user-land module loaders to compile and load builtins. diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index be4bedeb12a535..e135b1f798fa5e 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -439,7 +439,7 @@ Module.isBuiltin = BuiltinModule.isBuiltin; function initializeCJS() { // This need to be done at runtime in case --expose-internals is set. - let modules = Module.builtinModules = BuiltinModule.getAllBuiltinModuleIds(); + let modules = Module.builtinModules = BuiltinModule.getAllPublicBuiltinModuleIds(); if (!getOptionValue('--experimental-quic')) { modules = modules.filter((i) => i !== 'node:quic'); } diff --git a/test/parallel/test-module-builtin.js b/test/parallel/test-module-builtin.js index 3897d71ecf4405..c32fd535c1a516 100644 --- a/test/parallel/test-module-builtin.js +++ b/test/parallel/test-module-builtin.js @@ -12,3 +12,10 @@ assert.deepStrictEqual( builtinModules.filter((mod) => mod.startsWith('internal/')), [] ); + +// Does not include modules starting with an underscore +// (these are exposed to users but not proper public documented modules) +assert.deepStrictEqual( + builtinModules.filter((mod) => mod.startsWith('_')), + [] +); diff --git a/test/parallel/test-module-isBuiltin.js b/test/parallel/test-module-isBuiltin.js index a7815a8dfc1c18..7e5256d0a7ed61 100644 --- a/test/parallel/test-module-isBuiltin.js +++ b/test/parallel/test-module-isBuiltin.js @@ -14,3 +14,21 @@ assert(!isBuiltin('internal/errors')); assert(!isBuiltin('test')); assert(!isBuiltin('')); assert(!isBuiltin(undefined)); + +// Does not include modules starting with underscore +// (these can be required for historical reasons but +// are not proper documented public modules) +assert(!isBuiltin('_http_agent')); +assert(!isBuiltin('_http_client')); +assert(!isBuiltin('_http_common')); +assert(!isBuiltin('_http_incoming')); +assert(!isBuiltin('_http_outgoing')); +assert(!isBuiltin('_http_server')); +assert(!isBuiltin('_stream_duplex')); +assert(!isBuiltin('_stream_passthrough')); +assert(!isBuiltin('_stream_readable')); +assert(!isBuiltin('_stream_transform')); +assert(!isBuiltin('_stream_wrap')); +assert(!isBuiltin('_stream_writable')); +assert(!isBuiltin('_tls_common')); +assert(!isBuiltin('_tls_wrap')); From 021fd33297e0b7c83625d0bcebb1878268c8ff73 Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Wed, 19 Mar 2025 00:28:33 +0000 Subject: [PATCH 2/6] add comments to `isBuiltin` function --- lib/internal/bootstrap/realm.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/internal/bootstrap/realm.js b/lib/internal/bootstrap/realm.js index 79c7e234e7711d..5c65d1402262b6 100644 --- a/lib/internal/bootstrap/realm.js +++ b/lib/internal/bootstrap/realm.js @@ -314,18 +314,27 @@ class BuiltinModule { return false; } + // Modules starting with underscore (e.g. `_http_agent`) can be required + // by users but those are discouraged and should not be exposed as proper + // public ones, so if `id` starts with `_` return `false` right away if (StringPrototypeStartsWith(id, '_')) { return false; } + // Check if `id` matches a builtin module without the `node:` prefix if (BuiltinModule.canBeRequiredWithoutScheme(id)) { return true; } - return ( + // Check if `id` matches a builtin module with the `node:` prefix + if ( StringPrototypeStartsWith(id, 'node:') && BuiltinModule.canBeRequiredByUsers(StringPrototypeSlice(id, 5)) - ); + ) { + return true; + } + + return false; } static getSchemeOnlyModuleNames() { From 5811d08a521c68c208b590aff5795157eb33e76a Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Sat, 22 Mar 2025 19:12:49 +0000 Subject: [PATCH 3/6] Update lib/internal/bootstrap/realm.js Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com> --- lib/internal/bootstrap/realm.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/internal/bootstrap/realm.js b/lib/internal/bootstrap/realm.js index 5c65d1402262b6..996f1e120a5824 100644 --- a/lib/internal/bootstrap/realm.js +++ b/lib/internal/bootstrap/realm.js @@ -317,7 +317,8 @@ class BuiltinModule { // Modules starting with underscore (e.g. `_http_agent`) can be required // by users but those are discouraged and should not be exposed as proper // public ones, so if `id` starts with `_` return `false` right away - if (StringPrototypeStartsWith(id, '_')) { + if (StringPrototypeStartsWith(id, '_') || + StringPrototypeStartsWith(id, 'node:_') { return false; } From b252ffa59ef6ac78a11ebd5d40ea63b683dcfc66 Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Sat, 22 Mar 2025 19:15:28 +0000 Subject: [PATCH 4/6] fix incorrectly updated code --- lib/internal/bootstrap/realm.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/internal/bootstrap/realm.js b/lib/internal/bootstrap/realm.js index 996f1e120a5824..6a9f22c2e51683 100644 --- a/lib/internal/bootstrap/realm.js +++ b/lib/internal/bootstrap/realm.js @@ -316,9 +316,8 @@ class BuiltinModule { // Modules starting with underscore (e.g. `_http_agent`) can be required // by users but those are discouraged and should not be exposed as proper - // public ones, so if `id` starts with `_` return `false` right away - if (StringPrototypeStartsWith(id, '_') || - StringPrototypeStartsWith(id, 'node:_') { + // public ones, so if `id` starts with `_` (or `node:_`) return `false` right away + if (StringPrototypeStartsWith(id, '_') || StringPrototypeStartsWith(id, 'node:_')) { return false; } From a713daee1b6a092daaffccdd39c76e9e13e67942 Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Sat, 22 Mar 2025 19:21:03 +0000 Subject: [PATCH 5/6] update test to include `node:_*` modules --- test/parallel/test-module-isBuiltin.js | 36 ++++++++++++++++---------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/test/parallel/test-module-isBuiltin.js b/test/parallel/test-module-isBuiltin.js index 7e5256d0a7ed61..b302fbb0e5f43b 100644 --- a/test/parallel/test-module-isBuiltin.js +++ b/test/parallel/test-module-isBuiltin.js @@ -15,20 +15,28 @@ assert(!isBuiltin('test')); assert(!isBuiltin('')); assert(!isBuiltin(undefined)); +const underscoreModules = [ + '_http_agent', + '_http_client', + '_http_common', + '_http_incoming', + '_http_outgoing', + '_http_server', + '_stream_duplex', + '_stream_passthrough', + '_stream_readable', + '_stream_transform', + '_stream_wrap', + '_stream_writable', + '_tls_common', + '_tls_wrap', + 'node:_http_agent', +]; + // Does not include modules starting with underscore // (these can be required for historical reasons but // are not proper documented public modules) -assert(!isBuiltin('_http_agent')); -assert(!isBuiltin('_http_client')); -assert(!isBuiltin('_http_common')); -assert(!isBuiltin('_http_incoming')); -assert(!isBuiltin('_http_outgoing')); -assert(!isBuiltin('_http_server')); -assert(!isBuiltin('_stream_duplex')); -assert(!isBuiltin('_stream_passthrough')); -assert(!isBuiltin('_stream_readable')); -assert(!isBuiltin('_stream_transform')); -assert(!isBuiltin('_stream_wrap')); -assert(!isBuiltin('_stream_writable')); -assert(!isBuiltin('_tls_common')); -assert(!isBuiltin('_tls_wrap')); +for (const module of underscoreModules) { + assert(!isBuiltin(module)); + assert(!isBuiltin(`node:${module}`)); +} From fbeea6aa26b714854a4540ddf5e755144d76f252 Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Sat, 22 Mar 2025 22:56:00 +0000 Subject: [PATCH 6/6] fixup! update test to include `node:_*` modules remove erroneous module added to array --- test/parallel/test-module-isBuiltin.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/parallel/test-module-isBuiltin.js b/test/parallel/test-module-isBuiltin.js index b302fbb0e5f43b..af45f881c74c7f 100644 --- a/test/parallel/test-module-isBuiltin.js +++ b/test/parallel/test-module-isBuiltin.js @@ -30,7 +30,6 @@ const underscoreModules = [ '_stream_writable', '_tls_common', '_tls_wrap', - 'node:_http_agent', ]; // Does not include modules starting with underscore