From d9ddc2db6a0e6bf8c803990c9d229fea6827f8d9 Mon Sep 17 00:00:00 2001 From: Naz Date: Mon, 28 Jun 2021 14:25:29 +0400 Subject: [PATCH] Added extra validation for some settings of array type refs https://github.com/TryGhost/Team/issues/754 refs https://github.com/TryGhost/Ghost/commit/a7dec233ba5c9b289a981a157e9de074369272bd - Additional validation protects from problems like the ones in refed commit from even getting through to the database. - At the moment only used notificatons and couple more settings to ensure they are arrays when passed into the API. This is to avoid making big change in settings straight away - this is a problematic area which needs cautious approach. - Ideally in the future the list of settings to check the "array" type (and other types) should be automatically generated based on the default-settings.json (or whatever way we define settings in the db a that moment) - There's an ugly code-tripplication going on in this change. This is a separate topic that will be addressed once we work on API cleanup. --- .../canary/utils/validators/input/settings.js | 26 ++++++++++++++++++- .../api/v2/utils/validators/input/settings.js | 24 +++++++++++++++++ .../api/v3/utils/validators/input/settings.js | 26 ++++++++++++++++++- .../api/canary/admin/settings_spec.js | 17 +++++++++++- test/regression/api/v2/admin/settings_spec.js | 15 +++++++++++ test/regression/api/v3/admin/settings_spec.js | 15 +++++++++++ 6 files changed, 120 insertions(+), 3 deletions(-) diff --git a/core/server/api/canary/utils/validators/input/settings.js b/core/server/api/canary/utils/validators/input/settings.js index 5674e8e1ae..c5f9c590d0 100644 --- a/core/server/api/canary/utils/validators/input/settings.js +++ b/core/server/api/canary/utils/validators/input/settings.js @@ -1,7 +1,7 @@ const Promise = require('bluebird'); const _ = require('lodash'); const i18n = require('../../../../../../shared/i18n'); -const {NotFoundError} = require('@tryghost/errors'); +const {NotFoundError, ValidationError} = require('@tryghost/errors'); module.exports = { read(apiConfig, frame) { @@ -27,6 +27,30 @@ module.exports = { }) })); } + + // TODO: the below array is INCOMPLETE + // it should include all setting values that have array as a type + const arrayTypeSettings = [ + 'notifications', + 'navigation', + 'secondary_navigation' + ]; + + if (arrayTypeSettings.includes(setting.key)) { + const typeError = new ValidationError({ + message: `Value in ${setting.key} should be an array.`, + property: 'value' + }); + + try { + const value = JSON.parse(setting.value); + if (!_.isArray(value)) { + errors.push(typeError); + } + } catch (err) { + errors.push(typeError); + } + } }); if (errors.length) { diff --git a/core/server/api/v2/utils/validators/input/settings.js b/core/server/api/v2/utils/validators/input/settings.js index 50237bed36..fa039d4763 100644 --- a/core/server/api/v2/utils/validators/input/settings.js +++ b/core/server/api/v2/utils/validators/input/settings.js @@ -33,6 +33,30 @@ module.exports = { ); } } + + // TODO: the below array is INCOMPLETE + // it should include all setting values that have array as a type + const arrayTypeSettings = [ + 'notifications', + 'navigation', + 'secondary_navigation' + ]; + + if (arrayTypeSettings.includes(setting.key)) { + const typeError = new ValidationError({ + message: `Value in ${setting.key} should be an array.`, + property: 'value' + }); + + try { + const value = JSON.parse(setting.value); + if (!_.isArray(value)) { + errors.push(typeError); + } + } catch (err) { + errors.push(typeError); + } + } }); if (errors.length) { diff --git a/core/server/api/v3/utils/validators/input/settings.js b/core/server/api/v3/utils/validators/input/settings.js index 5674e8e1ae..c5f9c590d0 100644 --- a/core/server/api/v3/utils/validators/input/settings.js +++ b/core/server/api/v3/utils/validators/input/settings.js @@ -1,7 +1,7 @@ const Promise = require('bluebird'); const _ = require('lodash'); const i18n = require('../../../../../../shared/i18n'); -const {NotFoundError} = require('@tryghost/errors'); +const {NotFoundError, ValidationError} = require('@tryghost/errors'); module.exports = { read(apiConfig, frame) { @@ -27,6 +27,30 @@ module.exports = { }) })); } + + // TODO: the below array is INCOMPLETE + // it should include all setting values that have array as a type + const arrayTypeSettings = [ + 'notifications', + 'navigation', + 'secondary_navigation' + ]; + + if (arrayTypeSettings.includes(setting.key)) { + const typeError = new ValidationError({ + message: `Value in ${setting.key} should be an array.`, + property: 'value' + }); + + try { + const value = JSON.parse(setting.value); + if (!_.isArray(value)) { + errors.push(typeError); + } + } catch (err) { + errors.push(typeError); + } + } }); if (errors.length) { diff --git a/test/regression/api/canary/admin/settings_spec.js b/test/regression/api/canary/admin/settings_spec.js index f5e63aaff0..fb815f7b96 100644 --- a/test/regression/api/canary/admin/settings_spec.js +++ b/test/regression/api/canary/admin/settings_spec.js @@ -1227,10 +1227,25 @@ describe('Settings API (canary)', function () { dbSettings.should.have.property('twitter_image', '__GHOST_URL__/content/images/twitter_image.png'); }); + it('Can only send array values for keys defined with array type', async function () { + const settingsToChange = { + settings: [ + {key: 'navigation', value: 'not an array'} + ] + }; + + await request.put(localUtils.API.getApiQuery('settings/')) + .set('Origin', config.get('url')) + .send(settingsToChange) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(422); + }); + it('Cannot edit notifications key through API', async function () { const settingsToChange = { settings: [ - {key: 'notifications', value: `anything`} + {key: 'notifications', value: JSON.stringify(['do not touch me'])} ] }; diff --git a/test/regression/api/v2/admin/settings_spec.js b/test/regression/api/v2/admin/settings_spec.js index 14e6dd9651..5ef0032be9 100644 --- a/test/regression/api/v2/admin/settings_spec.js +++ b/test/regression/api/v2/admin/settings_spec.js @@ -708,6 +708,21 @@ describe('Settings API (v2)', function () { localUtils.API.checkResponse(putBody, 'settings'); }); + + it('Can only send array values for keys defined with array type', async function () { + const settingsToChange = { + settings: [ + {key: 'navigation', value: 'not an array'} + ] + }; + + await request.put(localUtils.API.getApiQuery('settings/')) + .set('Origin', config.get('url')) + .send(settingsToChange) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(422); + }); }); describe('As Admin', function () { diff --git a/test/regression/api/v3/admin/settings_spec.js b/test/regression/api/v3/admin/settings_spec.js index e78ad66941..4b50b8b905 100644 --- a/test/regression/api/v3/admin/settings_spec.js +++ b/test/regression/api/v3/admin/settings_spec.js @@ -702,6 +702,21 @@ describe('Settings API (v3)', function () { putBody.settings[0].key.should.eql('slack_username'); putBody.settings[0].value.should.eql('can edit me'); }); + + it('Can only send array values for keys defined with array type', async function () { + const settingsToChange = { + settings: [ + {key: 'navigation', value: 'not an array'} + ] + }; + + await request.put(localUtils.API.getApiQuery('settings/')) + .set('Origin', config.get('url')) + .send(settingsToChange) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(422); + }); }); describe('As Admin', function () {