From c0c04563198b9d8bc4e8b9d04a653330558bdf15 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 21 Oct 2022 23:45:48 +0200 Subject: [PATCH 1/2] fix: handle null or undefined hooks --- fastify.js | 4 ++++ test/hooks.test.js | 18 +++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/fastify.js b/fastify.js index c50ddf2c9e..366353fac7 100644 --- a/fastify.js +++ b/fastify.js @@ -566,6 +566,10 @@ function fastify (options) { function addHook (name, fn) { throwIfAlreadyStarted('Cannot call "addHook" when fastify instance is already started!') + if (fn == null) { + throw new errorCodes.FST_ERR_HOOK_INVALID_HANDLER(name, fn) + } + if (name === 'onSend' || name === 'preSerialization' || name === 'onError' || name === 'preParsing') { if (fn.constructor.name === 'AsyncFunction' && fn.length === 4) { throw new Error('Async function has too many arguments. Async hooks should not use the \'done\' argument.') diff --git a/test/hooks.test.js b/test/hooks.test.js index 75adf3be51..ceaf5ada4d 100644 --- a/test/hooks.test.js +++ b/test/hooks.test.js @@ -23,7 +23,7 @@ function getUrl (app) { } test('hooks', t => { - t.plan(43) + t.plan(49) const fastify = Fastify({ exposeHeadRoutes: false }) try { @@ -41,6 +41,22 @@ test('hooks', t => { t.fail() } + try { + fastify.addHook('preHandler', null) + } catch (e) { + t.equal(e.code, 'FST_ERR_HOOK_INVALID_HANDLER') + t.equal(e.message, 'preHandler hook should be a function, instead got null') + t.pass() + } + + try { + fastify.addHook('preParsing') + } catch (e) { + t.equal(e.code, 'FST_ERR_HOOK_INVALID_HANDLER') + t.equal(e.message, 'preParsing hook should be a function, instead got undefined') + t.pass() + } + try { fastify.addHook('preParsing', function (request, reply, payload, done) { request.preParsing = true From 0e3b135985d946f45cd882838d754c0f218995f2 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 21 Oct 2022 23:49:38 +0200 Subject: [PATCH 2/2] refactor: replace generic error for one with code --- fastify.js | 6 +++--- lib/errors.js | 6 ++++++ test/hooks-async.test.js | 8 ++++++-- test/hooks.on-ready.test.js | 3 ++- 4 files changed, 17 insertions(+), 6 deletions(-) diff --git a/fastify.js b/fastify.js index 366353fac7..eb9ce97cda 100644 --- a/fastify.js +++ b/fastify.js @@ -572,15 +572,15 @@ function fastify (options) { if (name === 'onSend' || name === 'preSerialization' || name === 'onError' || name === 'preParsing') { if (fn.constructor.name === 'AsyncFunction' && fn.length === 4) { - throw new Error('Async function has too many arguments. Async hooks should not use the \'done\' argument.') + throw new errorCodes.FST_ERR_HOOK_INVALID_ASYNC_HANDLER() } } else if (name === 'onReady') { if (fn.constructor.name === 'AsyncFunction' && fn.length !== 0) { - throw new Error('Async function has too many arguments. Async hooks should not use the \'done\' argument.') + throw new errorCodes.FST_ERR_HOOK_INVALID_ASYNC_HANDLER() } } else { if (fn.constructor.name === 'AsyncFunction' && fn.length === 3) { - throw new Error('Async function has too many arguments. Async hooks should not use the \'done\' argument.') + throw new errorCodes.FST_ERR_HOOK_INVALID_ASYNC_HANDLER() } } diff --git a/lib/errors.js b/lib/errors.js index f34b21481c..3ef8e568a4 100644 --- a/lib/errors.js +++ b/lib/errors.js @@ -101,6 +101,12 @@ const codes = { 500, TypeError ), + FST_ERR_HOOK_INVALID_ASYNC_HANDLER: createError( + 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER', + 'Async function has too many arguments. Async hooks should not use the \'done\' argument.', + 500, + TypeError + ), /** * Middlewares diff --git a/test/hooks-async.test.js b/test/hooks-async.test.js index 6240d8cd6b..8d801ba14f 100644 --- a/test/hooks-async.test.js +++ b/test/hooks-async.test.js @@ -585,33 +585,37 @@ test('preHandler respond with a stream', t => { test('Should log a warning if is an async function with `done`', t => { t.test('3 arguments', t => { - t.plan(1) + t.plan(2) const fastify = Fastify() try { fastify.addHook('onRequest', async (req, reply, done) => {}) } catch (e) { + t.ok(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER') t.ok(e.message === 'Async function has too many arguments. Async hooks should not use the \'done\' argument.') } }) t.test('4 arguments', t => { - t.plan(3) + t.plan(6) const fastify = Fastify() try { fastify.addHook('onSend', async (req, reply, payload, done) => {}) } catch (e) { + t.ok(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER') t.ok(e.message === 'Async function has too many arguments. Async hooks should not use the \'done\' argument.') } try { fastify.addHook('preSerialization', async (req, reply, payload, done) => {}) } catch (e) { + t.ok(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER') t.ok(e.message === 'Async function has too many arguments. Async hooks should not use the \'done\' argument.') } try { fastify.addHook('onError', async (req, reply, payload, done) => {}) } catch (e) { + t.ok(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER') t.ok(e.message === 'Async function has too many arguments. Async hooks should not use the \'done\' argument.') } }) diff --git a/test/hooks.on-ready.test.js b/test/hooks.on-ready.test.js index 42c241742a..034182b264 100644 --- a/test/hooks.on-ready.test.js +++ b/test/hooks.on-ready.test.js @@ -291,12 +291,13 @@ t.test('onReady cannot add lifecycle hooks', t => { }) t.test('onReady throw loading error', t => { - t.plan(1) + t.plan(2) const fastify = Fastify() try { fastify.addHook('onReady', async function (done) {}) } catch (e) { + t.ok(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER') t.ok(e.message === 'Async function has too many arguments. Async hooks should not use the \'done\' argument.') } })