From 5e9c197d85b79fc19425a5d26dbd09ecaf62e148 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Fri, 5 Aug 2022 00:10:13 +0200 Subject: [PATCH] esm: fix loader hooks accepting too many arguments PR-URL: https://github.com/nodejs/node/pull/44109 Reviewed-By: Geoffrey Booth Reviewed-By: Guy Bedford Reviewed-By: Antoine du Hamel --- lib/internal/modules/esm/loader.js | 22 +++++---------- test/es-module/test-esm-loader-chaining.mjs | 22 +++++++++++++++ .../es-module-loaders/loader-log-args.mjs | 28 +++++++++++++++++++ .../loader-with-too-many-args.mjs | 7 +++++ 4 files changed, 64 insertions(+), 15 deletions(-) create mode 100644 test/fixtures/es-module-loaders/loader-log-args.mjs create mode 100644 test/fixtures/es-module-loaders/loader-with-too-many-args.mjs diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 027077cbb6e781..f2d5600fba657c 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -15,7 +15,6 @@ const { ObjectDefineProperty, ObjectSetPrototypeOf, PromiseAll, - ReflectApply, RegExpPrototypeExec, SafeArrayIterator, SafeWeakMap, @@ -148,29 +147,22 @@ function nextHookFactory(chain, meta, { validateArgs, validateOutput }) { } return ObjectDefineProperty( - async (...args) => { + async (arg0 = undefined, context) => { // Update only when hook is invoked to avoid fingering the wrong filePath meta.hookErrIdentifier = `${hookFilePath} '${hookName}'`; - validateArgs(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, args); + validateArgs(`${meta.hookErrIdentifier} hook's ${nextHookName}()`, arg0, context); const outputErrIdentifier = `${chain[generatedHookIndex].url} '${hookName}' hook's ${nextHookName}()`; // Set when next is actually called, not just generated. if (generatedHookIndex === 0) { meta.chainFinished = true; } - // `context` is an optional argument that only needs to be passed when changed - switch (args.length) { - case 1: // It was omitted, so supply the cached value - ArrayPrototypePush(args, meta.context); - break; - case 2: // Overrides were supplied, so update cached value - ObjectAssign(meta.context, args[1]); - break; + if (context) { // `context` has already been validated, so no fancy check needed. + ObjectAssign(meta.context, context); } - ArrayPrototypePush(args, nextNextHook); - const output = await ReflectApply(hook, undefined, args); + const output = await hook(arg0, meta.context, nextNextHook); validateOutput(outputErrIdentifier, output); @@ -575,7 +567,7 @@ class ESMLoader { shortCircuited: false, }; - const validateArgs = (hookErrIdentifier, { 0: nextUrl, 1: ctx }) => { + const validateArgs = (hookErrIdentifier, nextUrl, ctx) => { if (typeof nextUrl !== 'string') { // non-strings can be coerced to a url string // validateString() throws a less-specific error @@ -829,7 +821,7 @@ class ESMLoader { shortCircuited: false, }; - const validateArgs = (hookErrIdentifier, { 0: suppliedSpecifier, 1: ctx }) => { + const validateArgs = (hookErrIdentifier, suppliedSpecifier, ctx) => { validateString( suppliedSpecifier, `${hookErrIdentifier} specifier`, diff --git a/test/es-module/test-esm-loader-chaining.mjs b/test/es-module/test-esm-loader-chaining.mjs index 14303cb5c42665..b04dbe4ddd6c1a 100644 --- a/test/es-module/test-esm-loader-chaining.mjs +++ b/test/es-module/test-esm-loader-chaining.mjs @@ -101,6 +101,28 @@ describe('ESM: loader chaining', { concurrency: true }, () => { assert.strictEqual(code, 0); }); + it('should accept only the correct arguments', async () => { + const { stdout } = await spawnPromisified( + execPath, + [ + '--loader', + fixtures.fileURL('es-module-loaders', 'loader-log-args.mjs'), + '--loader', + fixtures.fileURL('es-module-loaders', 'loader-with-too-many-args.mjs'), + ...commonArgs, + ], + { encoding: 'utf8' }, + ); + + assert.match(stdout, /^resolve arg count: 3$/m); + assert.match(stdout, /specifier: 'node:fs'/); + assert.match(stdout, /next: \[AsyncFunction: nextResolve\]/); + + assert.match(stdout, /^load arg count: 3$/m); + assert.match(stdout, /url: 'node:fs'/); + assert.match(stdout, /next: \[AsyncFunction: nextLoad\]/); + }); + it('should result in proper output from multiple changes in resolve hooks', async () => { const { code, stderr, stdout } = await spawnPromisified( execPath, diff --git a/test/fixtures/es-module-loaders/loader-log-args.mjs b/test/fixtures/es-module-loaders/loader-log-args.mjs new file mode 100644 index 00000000000000..84ed373d6b4de4 --- /dev/null +++ b/test/fixtures/es-module-loaders/loader-log-args.mjs @@ -0,0 +1,28 @@ +export async function resolve(...args) { + console.log(`resolve arg count: ${args.length}`); + console.log({ + specifier: args[0], + context: args[1], + next: args[2], + }); + + return { + shortCircuit: true, + url: args[0], + }; +} + +export async function load(...args) { + console.log(`load arg count: ${args.length}`); + console.log({ + url: args[0], + context: args[1], + next: args[2], + }); + + return { + format: 'module', + source: '', + shortCircuit: true, + }; +} diff --git a/test/fixtures/es-module-loaders/loader-with-too-many-args.mjs b/test/fixtures/es-module-loaders/loader-with-too-many-args.mjs new file mode 100644 index 00000000000000..95f40ec15d200d --- /dev/null +++ b/test/fixtures/es-module-loaders/loader-with-too-many-args.mjs @@ -0,0 +1,7 @@ +export async function resolve(specifier, context, next) { + return next(specifier, context, 'resolve-extra-arg'); +} + +export async function load(url, context, next) { + return next(url, context, 'load-extra-arg'); +}