From 275f3d85637d1d6342f0caf363c97b22e19273b9 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 28 Dec 2023 01:17:00 +0100 Subject: [PATCH 01/11] make file names kebab case --- index.js | 2 +- ...verifyBearerAuthFactory.js => verify-bearer-auth-factory.js} | 0 ...{decorateWithLogger.test.js => decorate-with-logger.test.js} | 0 ...erAuthFactory.test.js => verify-bearer-auth-factory.test.js} | 2 +- 4 files changed, 2 insertions(+), 2 deletions(-) rename lib/{verifyBearerAuthFactory.js => verify-bearer-auth-factory.js} (100%) rename test/{decorateWithLogger.test.js => decorate-with-logger.test.js} (100%) rename test/{verifyBearerAuthFactory.test.js => verify-bearer-auth-factory.test.js} (99%) diff --git a/index.js b/index.js index 5200be7..b1fb00a 100644 --- a/index.js +++ b/index.js @@ -1,7 +1,7 @@ 'use strict' const fp = require('fastify-plugin') -const verifyBearerAuthFactory = require('./lib/verifyBearerAuthFactory') +const verifyBearerAuthFactory = require('./lib/verify-bearer-auth-factory') function fastifyBearerAuth (fastify, options, done) { const defaultLogLevel = 'error' diff --git a/lib/verifyBearerAuthFactory.js b/lib/verify-bearer-auth-factory.js similarity index 100% rename from lib/verifyBearerAuthFactory.js rename to lib/verify-bearer-auth-factory.js diff --git a/test/decorateWithLogger.test.js b/test/decorate-with-logger.test.js similarity index 100% rename from test/decorateWithLogger.test.js rename to test/decorate-with-logger.test.js diff --git a/test/verifyBearerAuthFactory.test.js b/test/verify-bearer-auth-factory.test.js similarity index 99% rename from test/verifyBearerAuthFactory.test.js rename to test/verify-bearer-auth-factory.test.js index b7f5f1d..a3cf153 100644 --- a/test/verifyBearerAuthFactory.test.js +++ b/test/verify-bearer-auth-factory.test.js @@ -2,7 +2,7 @@ const test = require('tap').test const noop = () => {} -const verifyBearerAuthFactory = require('../lib/verifyBearerAuthFactory') +const verifyBearerAuthFactory = require('../lib/verify-bearer-auth-factory') const key = '123456789012354579814' const keys = { keys: new Set([key]) } From d4a95f19d1e319cf59135e691f4af4cc67659dfd Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 28 Dec 2023 01:18:59 +0100 Subject: [PATCH 02/11] add example for autocannon performance test --- examples/example.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 examples/example.js diff --git a/examples/example.js b/examples/example.js new file mode 100644 index 0000000..982a3a8 --- /dev/null +++ b/examples/example.js @@ -0,0 +1,21 @@ +'use strict' + +const fastify = require('fastify')({ +}) +const bearerAuthPlugin = require('..') +const keys = new Set(['key']) + +fastify.register(bearerAuthPlugin, { keys }) +fastify.get('/foo', (req, reply) => { + reply.send({ authenticated: true }) +}) + +fastify.listen({ port: 8000 }, (err) => { + if (err) { + fastify.log.error(err.message) + process.exit(1) + } + fastify.log.info('http://127.0.0.1:8000/foo') +}) + +// autocannon -H authorization='Bearer key' http://127.0.0.1:8000/foo From e67a0512d31e88bf0d410922ebb35f0d87509452 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 28 Dec 2023 01:25:34 +0100 Subject: [PATCH 03/11] instantiate error only when needed --- lib/verify-bearer-auth-factory.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/verify-bearer-auth-factory.js b/lib/verify-bearer-auth-factory.js index 7cecfa7..64c462b 100644 --- a/lib/verify-bearer-auth-factory.js +++ b/lib/verify-bearer-auth-factory.js @@ -48,7 +48,7 @@ module.exports = function verifyBearerAuthFactory (options) { } const key = header.substring(bearerType.length).trim() - let retVal + let retVal = false // check if auth function is defined if (auth && auth instanceof Function) { try { @@ -62,17 +62,16 @@ module.exports = function verifyBearerAuthFactory (options) { retVal = authenticate(keys, key) } - const invalidKeyError = Error('invalid authorization header') - // retVal contains the result of the auth function if defined or the // result of the key comparison. // retVal is enclosed in a Promise.resolve to allow auth to be a normal - // function or an async funtion. If it returns a non-promise value it + // function or an async function. If it returns a non-promise value it // will be converted to a resolving promise. If it returns a promise it // will be resolved. Promise.resolve(retVal).then((val) => { // if val is not truthy return 401 if (val === false) { + const invalidKeyError = Error('invalid authorization header') if (verifyErrorLogLevel) request.log[verifyErrorLogLevel]('unauthorized: %s', invalidKeyError.message) if (contentType) reply.header('content-type', contentType) reply.code(401) From af2f15a16744a46e000dce9749074a706344fb49 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 28 Dec 2023 01:38:52 +0100 Subject: [PATCH 04/11] rename header to authorizationHeader --- lib/verify-bearer-auth-factory.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/verify-bearer-auth-factory.js b/lib/verify-bearer-auth-factory.js index 64c462b..ba0b565 100644 --- a/lib/verify-bearer-auth-factory.js +++ b/lib/verify-bearer-auth-factory.js @@ -37,17 +37,17 @@ module.exports = function verifyBearerAuthFactory (options) { reply.send(errorResponse(noHeaderError)) } - const header = request.raw.headers.authorization - if (!header) { + const authorizationHeader = request.raw.headers.authorization + if (!authorizationHeader) { return authorizationHeaderErrorFn('missing authorization header') } - const type = header.substring(0, bearerType.length) + const type = authorizationHeader.substring(0, bearerType.length) if (type !== bearerType) { return authorizationHeaderErrorFn('invalid authorization header') } - const key = header.substring(bearerType.length).trim() + const key = authorizationHeader.substring(bearerType.length).trim() let retVal = false // check if auth function is defined if (auth && auth instanceof Function) { From 5681f77cb3c4f36805a7ad8d7b84a0debf1c3c51 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 28 Dec 2023 01:56:40 +0100 Subject: [PATCH 05/11] fix bug, when missing space between bearerType and key --- lib/verify-bearer-auth-factory.js | 9 ++++++--- test/integration.test.js | 16 ++++++++++++++++ test/verify-bearer-auth-factory.test.js | 23 +++++++++++++++++++++++ 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/lib/verify-bearer-auth-factory.js b/lib/verify-bearer-auth-factory.js index ba0b565..ed80299 100644 --- a/lib/verify-bearer-auth-factory.js +++ b/lib/verify-bearer-auth-factory.js @@ -24,6 +24,9 @@ module.exports = function verifyBearerAuthFactory (options) { keys[i] = Buffer.from(keys[i]) } + const bearerTypePrefix = bearerType + ' ' + const bearerTypePrefixLength = bearerType.length + 1 + return function verifyBearerAuth (request, reply, done) { function authorizationHeaderErrorFn (errorMessage) { const noHeaderError = Error(errorMessage) @@ -42,12 +45,12 @@ module.exports = function verifyBearerAuthFactory (options) { return authorizationHeaderErrorFn('missing authorization header') } - const type = authorizationHeader.substring(0, bearerType.length) - if (type !== bearerType) { + const type = authorizationHeader.substring(0, bearerTypePrefixLength) + if (type !== bearerTypePrefix) { return authorizationHeaderErrorFn('invalid authorization header') } - const key = authorizationHeader.substring(bearerType.length).trim() + const key = authorizationHeader.substring(bearerTypePrefixLength).trim() let retVal = false // check if auth function is defined if (auth && auth instanceof Function) { diff --git a/test/integration.test.js b/test/integration.test.js index d57e803..871c310 100644 --- a/test/integration.test.js +++ b/test/integration.test.js @@ -43,6 +43,22 @@ test('invalid key route fails correctly', (t) => { }) }) +test('missing space between bearerType and key fails correctly', (t) => { + t.plan(2) + fastify.inject({ + method: 'GET', + url: '/test', + headers: { + authorization: 'Bearer123456' + } + }).then(response => { + t.equal(response.statusCode, 401) + t.match(JSON.parse(response.body).error, /invalid authorization header/) + }).catch(err => { + t.error(err) + }) +}) + test('missing header route fails correctly', (t) => { t.plan(2) fastify.inject({ method: 'GET', url: '/test' }).then(response => { diff --git a/test/verify-bearer-auth-factory.test.js b/test/verify-bearer-auth-factory.test.js index a3cf153..cfade70 100644 --- a/test/verify-bearer-auth-factory.test.js +++ b/test/verify-bearer-auth-factory.test.js @@ -218,6 +218,29 @@ test('hook accepts correct header and alternate Bearer', (t) => { }) }) +test('hook throws if header misses at least one space after bearerType', (t) => { + t.plan(2) + + const request = { + log: { error: noop }, + raw: { + headers: { authorization: `Bearer${key}` } + } + } + const response = { + code: () => response, + send + } + + function send (body) { + t.ok(body.error) + t.match(body.error, /invalid authorization header/) + } + + const hook = verifyBearerAuthFactory(keys) + hook(request, response) +}) + test('hook accepts correct header with extra padding', (t) => { t.plan(1) From e442cff67b8b91d6d6c1939b55dd148789f284d7 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 28 Dec 2023 02:01:32 +0100 Subject: [PATCH 06/11] avoid intermediary variable --- lib/verify-bearer-auth-factory.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/verify-bearer-auth-factory.js b/lib/verify-bearer-auth-factory.js index ed80299..88e2d6b 100644 --- a/lib/verify-bearer-auth-factory.js +++ b/lib/verify-bearer-auth-factory.js @@ -45,8 +45,7 @@ module.exports = function verifyBearerAuthFactory (options) { return authorizationHeaderErrorFn('missing authorization header') } - const type = authorizationHeader.substring(0, bearerTypePrefixLength) - if (type !== bearerTypePrefix) { + if (authorizationHeader.substring(0, bearerTypePrefixLength) !== bearerTypePrefix) { return authorizationHeaderErrorFn('invalid authorization header') } From 96427b27ddb52046c412b5709060cec2816e4c0f Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 28 Dec 2023 02:03:24 +0100 Subject: [PATCH 07/11] document more autocannon cases in example --- examples/example.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/examples/example.js b/examples/example.js index 982a3a8..d923f3f 100644 --- a/examples/example.js +++ b/examples/example.js @@ -18,4 +18,9 @@ fastify.listen({ port: 8000 }, (err) => { fastify.log.info('http://127.0.0.1:8000/foo') }) +// Missing Header +// autocannon http://127.0.0.1:8000/foo +// Invalid Header +// autocannon -H authorization='Bearer invalid' http://127.0.0.1:8000/foo +// Valid Request // autocannon -H authorization='Bearer key' http://127.0.0.1:8000/foo From bba8984747ff3c408d570ba381272c551db40f3b Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 28 Dec 2023 02:06:54 +0100 Subject: [PATCH 08/11] rename authorizationHeaderErrorFn to handleUnauthorized --- lib/verify-bearer-auth-factory.js | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/lib/verify-bearer-auth-factory.js b/lib/verify-bearer-auth-factory.js index 88e2d6b..d7ba416 100644 --- a/lib/verify-bearer-auth-factory.js +++ b/lib/verify-bearer-auth-factory.js @@ -28,7 +28,7 @@ module.exports = function verifyBearerAuthFactory (options) { const bearerTypePrefixLength = bearerType.length + 1 return function verifyBearerAuth (request, reply, done) { - function authorizationHeaderErrorFn (errorMessage) { + function handleUnauthorized (errorMessage) { const noHeaderError = Error(errorMessage) if (verifyErrorLogLevel) request.log[verifyErrorLogLevel]('unauthorized: %s', noHeaderError.message) if (contentType) reply.header('content-type', contentType) @@ -42,11 +42,11 @@ module.exports = function verifyBearerAuthFactory (options) { const authorizationHeader = request.raw.headers.authorization if (!authorizationHeader) { - return authorizationHeaderErrorFn('missing authorization header') + return handleUnauthorized('missing authorization header') } if (authorizationHeader.substring(0, bearerTypePrefixLength) !== bearerTypePrefix) { - return authorizationHeaderErrorFn('invalid authorization header') + return handleUnauthorized('invalid authorization header') } const key = authorizationHeader.substring(bearerTypePrefixLength).trim() @@ -73,12 +73,7 @@ module.exports = function verifyBearerAuthFactory (options) { Promise.resolve(retVal).then((val) => { // if val is not truthy return 401 if (val === false) { - const invalidKeyError = Error('invalid authorization header') - if (verifyErrorLogLevel) request.log[verifyErrorLogLevel]('unauthorized: %s', invalidKeyError.message) - if (contentType) reply.header('content-type', contentType) - reply.code(401) - if (!addHook) return done(invalidKeyError) - reply.send(errorResponse(invalidKeyError)) + handleUnauthorized('invalid authorization header') return } if (val === true) { From e67d39538e8ec80f5e9c737c8d999c0360775aa1 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 28 Dec 2023 02:09:41 +0100 Subject: [PATCH 09/11] extract handleUnauthorized from verifyBearerAuth --- lib/verify-bearer-auth-factory.js | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/verify-bearer-auth-factory.js b/lib/verify-bearer-auth-factory.js index d7ba416..e498a56 100644 --- a/lib/verify-bearer-auth-factory.js +++ b/lib/verify-bearer-auth-factory.js @@ -27,26 +27,26 @@ module.exports = function verifyBearerAuthFactory (options) { const bearerTypePrefix = bearerType + ' ' const bearerTypePrefixLength = bearerType.length + 1 - return function verifyBearerAuth (request, reply, done) { - function handleUnauthorized (errorMessage) { - const noHeaderError = Error(errorMessage) - if (verifyErrorLogLevel) request.log[verifyErrorLogLevel]('unauthorized: %s', noHeaderError.message) - if (contentType) reply.header('content-type', contentType) - reply.code(401) - if (!addHook) { - done(noHeaderError) - return - } - reply.send(errorResponse(noHeaderError)) + function handleUnauthorized (request, reply, done, message) { + const noHeaderError = Error(message) + if (verifyErrorLogLevel) request.log[verifyErrorLogLevel]('unauthorized: %s', noHeaderError.message) + if (contentType) reply.header('content-type', contentType) + reply.code(401) + if (!addHook) { + done(noHeaderError) + return } + reply.send(errorResponse(noHeaderError)) + } + return function verifyBearerAuth (request, reply, done) { const authorizationHeader = request.raw.headers.authorization if (!authorizationHeader) { - return handleUnauthorized('missing authorization header') + return handleUnauthorized(request, reply, done, 'missing authorization header') } if (authorizationHeader.substring(0, bearerTypePrefixLength) !== bearerTypePrefix) { - return handleUnauthorized('invalid authorization header') + return handleUnauthorized(request, reply, done, 'invalid authorization header') } const key = authorizationHeader.substring(bearerTypePrefixLength).trim() @@ -73,7 +73,7 @@ module.exports = function verifyBearerAuthFactory (options) { Promise.resolve(retVal).then((val) => { // if val is not truthy return 401 if (val === false) { - handleUnauthorized('invalid authorization header') + handleUnauthorized(request, reply, done, 'invalid authorization header') return } if (val === true) { From fadfc337d8660043607a7acf85840f7cd46b0920 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 28 Dec 2023 02:22:48 +0100 Subject: [PATCH 10/11] add invalid bearertype case to example --- examples/example.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/examples/example.js b/examples/example.js index d923f3f..91e8502 100644 --- a/examples/example.js +++ b/examples/example.js @@ -20,7 +20,9 @@ fastify.listen({ port: 8000 }, (err) => { // Missing Header // autocannon http://127.0.0.1:8000/foo -// Invalid Header +// Invalid Bearer Type +// autocannon -H authorization='Beaver key' http://127.0.0.1:8000/foo +// Invalid Key // autocannon -H authorization='Bearer invalid' http://127.0.0.1:8000/foo // Valid Request // autocannon -H authorization='Bearer key' http://127.0.0.1:8000/foo From 8f1657b928daee96487baa3548c2f9876baea620 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 28 Dec 2023 02:57:32 +0100 Subject: [PATCH 11/11] remove unnecessary await --- test/integration.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration.test.js b/test/integration.test.js index 871c310..2211240 100644 --- a/test/integration.test.js +++ b/test/integration.test.js @@ -74,7 +74,7 @@ test('integration with @fastify/auth', async (t) => { const fastify = require('fastify')() await fastify.register(plugin, { addHook: false, keys: new Set(['123456']) }) - await fastify.decorate('allowAnonymous', function (request, _, done) { + fastify.decorate('allowAnonymous', function (request, _, done) { if (!request.headers.authorization) { return done() } @@ -135,7 +135,7 @@ test('integration with @fastify/auth; not the last auth option', async (t) => { const fastify = require('fastify')() await fastify.register(plugin, { addHook: false, keys: new Set(['123456']) }) - await fastify.decorate('alwaysValidAuth', function (request, _, done) { + fastify.decorate('alwaysValidAuth', function (request, _, done) { return done() }) await fastify.register(require('@fastify/auth'))