From 13099b4c552a7b67db80fb3b2f839f6785ca3d20 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Mon, 27 Feb 2023 17:56:23 +0100 Subject: [PATCH 1/3] Send error status when token salt not set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Jean-Sébastien Herbaux --- .../admin/server/middlewares/data-transfer.js | 26 +++++++++++++ .../core/admin/server/middlewares/index.js | 1 + packages/core/admin/server/routes/transfer.js | 16 ++++---- .../admin/server/services/transfer/index.js | 1 + .../admin/server/services/transfer/utils.js | 38 +++++++++++++++++++ 5 files changed, 73 insertions(+), 9 deletions(-) create mode 100644 packages/core/admin/server/middlewares/data-transfer.js create mode 100644 packages/core/admin/server/services/transfer/utils.js diff --git a/packages/core/admin/server/middlewares/data-transfer.js b/packages/core/admin/server/middlewares/data-transfer.js new file mode 100644 index 0000000000..3282324974 --- /dev/null +++ b/packages/core/admin/server/middlewares/data-transfer.js @@ -0,0 +1,26 @@ +'use strict'; + +const { getService } = require('../utils'); + +module.exports = () => async (ctx, next) => { + const transferUtils = getService('transfer').utils; + + const { hasValidTokenSalt, isDataTransferEnabled, isDisabledFromEnv } = transferUtils; + + if (isDataTransferEnabled()) { + return next(); + } + + if (!hasValidTokenSalt()) { + return ctx.notImplemented( + 'The server configuration for data transfer is invalid. Please contact your server administrator.' + ); + } + + if (isDisabledFromEnv()) { + return ctx.notFound(); + } + + // This should never happen as long as we're handling individual scenarios above + throw new Error('Unexpected error while trying to access a data transfer route'); +}; diff --git a/packages/core/admin/server/middlewares/index.js b/packages/core/admin/server/middlewares/index.js index 2abf96cbb1..e575f76216 100644 --- a/packages/core/admin/server/middlewares/index.js +++ b/packages/core/admin/server/middlewares/index.js @@ -4,4 +4,5 @@ const rateLimit = require('./rateLimit'); module.exports = { rateLimit, + 'data-transfer': require('./data-transfer'), }; diff --git a/packages/core/admin/server/routes/transfer.js b/packages/core/admin/server/routes/transfer.js index 7b88c927ce..a8f13c5fa4 100644 --- a/packages/core/admin/server/routes/transfer.js +++ b/packages/core/admin/server/routes/transfer.js @@ -9,15 +9,7 @@ module.exports = [ path: '/transfer/runner/connect', handler: 'transfer.runner-connect', config: { - middlewares: [ - (ctx, next) => { - if (process.env.STRAPI_DISABLE_REMOTE_DATA_TRANSFER === 'true') { - return ctx.notFound(); - } - - return next(); - }, - ], + middlewares: ['admin::data-transfer'], // TODO: Allow not passing any scope <> Add a way to prevent assigning one by default auth: { strategies: [dataTransferAuthStrategy], scope: ['push'] }, }, @@ -28,6 +20,7 @@ module.exports = [ path: '/transfer/tokens', handler: 'transfer.token-create', config: { + middlewares: ['admin::data-transfer'], policies: [ 'admin::isAuthenticatedAdmin', { name: 'admin::hasPermissions', config: { actions: ['admin::transfer.tokens.create'] } }, @@ -39,6 +32,7 @@ module.exports = [ path: '/transfer/tokens', handler: 'transfer.token-list', config: { + middlewares: ['admin::data-transfer'], policies: [ 'admin::isAuthenticatedAdmin', { name: 'admin::hasPermissions', config: { actions: ['admin::transfer.tokens.read'] } }, @@ -50,6 +44,7 @@ module.exports = [ path: '/transfer/tokens/:id', handler: 'transfer.token-revoke', config: { + middlewares: ['admin::data-transfer'], policies: [ 'admin::isAuthenticatedAdmin', { name: 'admin::hasPermissions', config: { actions: ['admin::transfer.tokens.delete'] } }, @@ -61,6 +56,7 @@ module.exports = [ path: '/transfer/tokens/:id', handler: 'transfer.token-getById', config: { + middlewares: ['admin::data-transfer'], policies: [ 'admin::isAuthenticatedAdmin', { name: 'admin::hasPermissions', config: { actions: ['admin::transfer.tokens.read'] } }, @@ -72,6 +68,7 @@ module.exports = [ path: '/transfer/tokens/:id', handler: 'transfer.token-update', config: { + middlewares: ['admin::data-transfer'], policies: [ 'admin::isAuthenticatedAdmin', { name: 'admin::hasPermissions', config: { actions: ['admin::transfer.tokens.update'] } }, @@ -83,6 +80,7 @@ module.exports = [ path: '/transfer/tokens/:id/regenerate', handler: 'transfer.token-regenerate', config: { + middlewares: ['admin::data-transfer'], policies: [ 'admin::isAuthenticatedAdmin', { diff --git a/packages/core/admin/server/services/transfer/index.js b/packages/core/admin/server/services/transfer/index.js index efd83a7d0b..7c67cc99fa 100644 --- a/packages/core/admin/server/services/transfer/index.js +++ b/packages/core/admin/server/services/transfer/index.js @@ -3,4 +3,5 @@ module.exports = { permission: require('./permission'), token: require('./token'), + utils: require('./utils'), }; diff --git a/packages/core/admin/server/services/transfer/utils.js b/packages/core/admin/server/services/transfer/utils.js new file mode 100644 index 0000000000..b393e91891 --- /dev/null +++ b/packages/core/admin/server/services/transfer/utils.js @@ -0,0 +1,38 @@ +'use strict'; + +const { env } = require('@strapi/utils'); + +const { getService } = require('../../utils'); + +/** + * Returns whether the data transfer features have been disabled from the env configuration + * + * @returns {boolean} + */ +const isDisabledFromEnv = () => { + return env.bool('STRAPI_DISABLE_REMOTE_DATA_TRANSFER', false); +}; + +/** + * A valid transfer token salt must be a non-empty string defined in the Strapi config + * + * @returns {boolean} + */ +const hasValidTokenSalt = () => { + const salt = strapi.config.get('TRANSFER_TOKEN_SALT', null); + + return typeof salt === 'string' && salt.length > 0; +}; + +/** + * Checks whether data transfer features are enabled + * + * @returns {boolean} + */ +const isDataTransferEnabled = () => { + const { utils } = getService('transfer'); + + return !utils.isDisabledFromEnv() && utils.hasValidTokenSalt(); +}; + +module.exports = { isDataTransferEnabled, isDisabledFromEnv, hasValidTokenSalt }; From ae23359954c1d393eb6d23a27450c6037cd074b4 Mon Sep 17 00:00:00 2001 From: Convly Date: Mon, 27 Feb 2023 18:55:16 +0100 Subject: [PATCH 2/3] Emit a warning when the feature is enabled but salt is not set --- .../core/admin/server/services/transfer/token.js | 15 ++++++++++++--- .../core/admin/server/services/transfer/utils.js | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/core/admin/server/services/transfer/token.js b/packages/core/admin/server/services/transfer/token.js index f3273da076..a422d3a1f2 100644 --- a/packages/core/admin/server/services/transfer/token.js +++ b/packages/core/admin/server/services/transfer/token.js @@ -8,6 +8,7 @@ const { } = require('@strapi/utils'); const constants = require('../constants'); +const { getService } = require('../../utils'); const TRANSFER_TOKEN_UID = 'admin::transfer-token'; const TRANSFER_TOKEN_PERMISSION_UID = 'admin::transfer-token-permission'; @@ -337,9 +338,17 @@ const hash = (accessKey) => { * @returns {void} */ const checkSaltIsDefined = () => { - if (!strapi.config.get('admin.transfer.token.salt')) { - throw new Error( - `Missing transfer.token.salt. Please set transfer.token.salt in config/admin.js (ex: you can generate one using Node with \`crypto.randomBytes(16).toString('base64')\`). + const { hasValidTokenSalt, isDisabledFromEnv } = getService('transfer').utils; + + // Ignore the check if the data-transfer feature is manually disabled + if (isDisabledFromEnv()) { + return; + } + + if (!hasValidTokenSalt()) { + process.emitWarning( + `Missing transfer.token.salt: Data transfer features have been disabled. +Please set transfer.token.salt in config/admin.js (ex: you can generate one using Node with \`crypto.randomBytes(16).toString('base64')\`) For security reasons, prefer storing the secret in an environment variable and read it in config/admin.js. See https://docs.strapi.io/developer-docs/latest/setup-deployment-guides/configurations/optional/environment.html#configuration-using-environment-variables.` ); } diff --git a/packages/core/admin/server/services/transfer/utils.js b/packages/core/admin/server/services/transfer/utils.js index b393e91891..715e89fef5 100644 --- a/packages/core/admin/server/services/transfer/utils.js +++ b/packages/core/admin/server/services/transfer/utils.js @@ -19,7 +19,7 @@ const isDisabledFromEnv = () => { * @returns {boolean} */ const hasValidTokenSalt = () => { - const salt = strapi.config.get('TRANSFER_TOKEN_SALT', null); + const salt = strapi.config.get('admin.transfer.token.salt', null); return typeof salt === 'string' && salt.length > 0; }; From 9afb0ad86d7c406457ed715d9be9a7848a5164f1 Mon Sep 17 00:00:00 2001 From: Convly Date: Mon, 27 Feb 2023 19:10:04 +0100 Subject: [PATCH 3/3] Check token salt on hash --- packages/core/admin/server/services/transfer/token.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/core/admin/server/services/transfer/token.js b/packages/core/admin/server/services/transfer/token.js index a422d3a1f2..95b7111442 100644 --- a/packages/core/admin/server/services/transfer/token.js +++ b/packages/core/admin/server/services/transfer/token.js @@ -328,6 +328,12 @@ const getExpirationFields = (lifespan) => { * @returns {string} */ const hash = (accessKey) => { + const { hasValidTokenSalt } = getService('transfer').utils; + + if (!hasValidTokenSalt()) { + throw new TypeError('Required token salt is not defined'); + } + return crypto .createHmac('sha512', strapi.config.get('admin.transfer.token.salt')) .update(accessKey)