-
-
Couldn't load subscription status.
- Fork 33.6k
module: fix ERR_INTERNAL_ASSERTION in CJS module loading #60408
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.
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
base: main
Are you sure you want to change the base?
Conversation
…_ASSERTION in CJS module loading Fixes: nodejs#60401 The loadCJSModule function was not properly validating source content before passing it to compileFunctionForCJSLoader, causing internal assertions when source was null or undefined. This change adds proper source validation and throws a meaningful ERR_INVALID_RETURN_PROPERTY_VALUE error instead of failing with an internal assertion.
|
Review requested:
|
Could you add a test? |
Add test cases to verify that loadCJSModule properly handles null, undefined, and empty string source values by throwing ERR_INVALID_RETURN_PROPERTY_VALUE instead of triggering ERR_INTERNAL_ASSERTION. Tests three scenarios: - Custom loader returning null source - Custom loader returning undefined source - Custom loader returning empty string source Refs: nodejs#60401
done, added test case in second commit to verify the fix works correctly. The test covers null, undefined, and empty string source values to ensure ERR_INVALID_RETURN_PROPERTY_VALUE is thrown instead of ERR_INTERNAL_ASSERTION. @aduh95 |
|
Friendly ping @nodejs/loaders - would appreciate any feedback when you have time! |
| try { | ||
| await import('file:///test-null-source.js'); | ||
| console.log('ERROR: Should have thrown'); | ||
| process.exit(1); | ||
| } catch (err) { | ||
| // Should throw ERR_INVALID_RETURN_PROPERTY_VALUE, not ERR_INTERNAL_ASSERTION | ||
| if (err.code === 'ERR_INTERNAL_ASSERTION') { | ||
| console.log('FAIL: Got ERR_INTERNAL_ASSERTION'); | ||
| process.exit(1); | ||
| } | ||
| if (err.code === 'ERR_INVALID_RETURN_PROPERTY_VALUE') { | ||
| console.log('PASS: Got expected error'); | ||
| process.exit(0); | ||
| } | ||
| console.log('ERROR: Got unexpected error:', err.code); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| try { | |
| await import('file:///test-null-source.js'); | |
| console.log('ERROR: Should have thrown'); | |
| process.exit(1); | |
| } catch (err) { | |
| // Should throw ERR_INVALID_RETURN_PROPERTY_VALUE, not ERR_INTERNAL_ASSERTION | |
| if (err.code === 'ERR_INTERNAL_ASSERTION') { | |
| console.log('FAIL: Got ERR_INTERNAL_ASSERTION'); | |
| process.exit(1); | |
| } | |
| if (err.code === 'ERR_INVALID_RETURN_PROPERTY_VALUE') { | |
| console.log('PASS: Got expected error'); | |
| process.exit(0); | |
| } | |
| console.log('ERROR: Got unexpected error:', err.code); | |
| process.exit(1); | |
| } | |
| await assert.rejects(import('file:///test-null-source.js'), { code: 'ERR_INVALID_RETURN_PROPERTY_VALUE' }); |
| const result = spawnSync( | ||
| process.execPath, | ||
| [ | ||
| '--no-warnings', | ||
| '--input-type=module', | ||
| '--eval', | ||
| ` | ||
| import { register } from 'node:module'; | ||
|
|
||
| // Register a custom loader that returns null source | ||
| const code = 'export function load(url, context, next) {' + | ||
| ' if (url.includes("test-null-source")) {' + | ||
| ' return { format: "commonjs", source: null, shortCircuit: true };' + | ||
| ' }' + | ||
| ' return next(url);' + | ||
| '}'; | ||
|
|
||
| register('data:text/javascript,' + encodeURIComponent(code)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's declare the function outside the string so it's easier to read and can be linted
| const result = spawnSync( | |
| process.execPath, | |
| [ | |
| '--no-warnings', | |
| '--input-type=module', | |
| '--eval', | |
| ` | |
| import { register } from 'node:module'; | |
| // Register a custom loader that returns null source | |
| const code = 'export function load(url, context, next) {' + | |
| ' if (url.includes("test-null-source")) {' + | |
| ' return { format: "commonjs", source: null, shortCircuit: true };' + | |
| ' }' + | |
| ' return next(url);' + | |
| '}'; | |
| register('data:text/javascript,' + encodeURIComponent(code)); | |
| function load(url, context, next) { | |
| if (url.includes("test-null-source")) { | |
| return { format: "commonjs", source: null, shortCircuit: true }; | |
| } | |
| return next(url); | |
| } | |
| const result = spawnSync( | |
| process.execPath, | |
| [ | |
| '--no-warnings', | |
| '--input-type=module', | |
| '--eval', | |
| ` | |
| import { register } from 'node:module'; | |
| // Register a custom loader that returns null source | |
| register('data:text/javascript,export ' + encodeURIComponent(${load})); |
Description
Fixes #60401
The
loadCJSModulefunction inlib/internal/modules/esm/translators.jswas not validating source content before passing it to the nativecompileFunctionForCJSLoaderfunction. When source wasnullorundefined, this caused an internal assertion failure instead of throwing a meaningful error.Changes
loadCJSModuleto check for null/undefined/empty sourceERR_INVALID_RETURN_PROPERTY_VALUEwith descriptive message instead of internal assertionTesting
The validation prevents the internal assertion that was occurring with custom loaders returning null source.
test/parallel/test-esm-loader-null-source.jsChecklist
make -j4 test(UNIX) orvcbuild test(Windows) passes (will be verified by CI)