From 8f660c32598f560027b3958e78f7b01223b482f0 Mon Sep 17 00:00:00 2001 From: Fabien 'egg' O'Carroll Date: Wed, 15 Jul 2020 17:11:27 +0200 Subject: [PATCH] Improved settings validation (#12048) closes #12001 * Moved settings validation to the model This moves the settings validation out of the validation file and into the model, as it is _only_ used there. It also sets us up in the future for custom validators on individual settings. * Improved validation of stripe_plans setting - Checks `interval` is a valid string - Checks `name` & `currency` are strings * Moved stripe key validation into model The stripe key settings are all nullable and the regex validation fails when the input is `null`. Rather than reworking the entirety of how we validate with default-settings validation objects, this moves the validation into methods on the Settings model. * Added tests for new setting validations Adds tests for both valid and invalid settings, as well as helpers making future tests easier and less repetitive --- core/server/data/validation/index.js | 34 +---- core/server/models/settings.js | 117 ++++++++++++++- test/unit/data/validation/index_spec.js | 3 +- test/unit/models/settings_spec.js | 190 ++++++++++++++++++++++++ 4 files changed, 308 insertions(+), 36 deletions(-) diff --git a/core/server/data/validation/index.js b/core/server/data/validation/index.js index be21d0be55..2fa11331ed 100644 --- a/core/server/data/validation/index.js +++ b/core/server/data/validation/index.js @@ -269,37 +269,6 @@ function validateSchema(tableName, model, options) { return Promise.resolve(); } -// Validation for settings -// settings are checked against the validation objects -// form default-settings.json -async function validateSettings(defaultSettings, model) { - const setting = model.toJSON(); - let validationErrors = []; - const matchingDefault = defaultSettings[setting.key]; - - if (matchingDefault && matchingDefault.validations) { - validationErrors = validationErrors.concat(validate(setting.value, setting.key, matchingDefault.validations, 'settings')); - } - - if (validationErrors.length !== 0) { - return Promise.reject(validationErrors); - } - - if (setting.key === 'stripe_plans') { - const plans = JSON.parse(setting.value); - for (const plan of plans) { - // We check 100, not 1, because amounts are in fractional units - if (plan.amount < 100 && plan.name !== 'Complimentary') { - throw new errors.ValidationError({ - message: 'Plans cannot have an amount less than 1' - }); - } - } - } - - return Promise.resolve(); -} - /** * Validate keys using the validator module. * Each validation's key is a method name and its value is an array of options @@ -370,6 +339,5 @@ module.exports = { validate, validator, validatePassword, - validateSchema, - validateSettings + validateSchema }; diff --git a/core/server/models/settings.js b/core/server/models/settings.js index 608ecc691d..f1afdda95d 100644 --- a/core/server/models/settings.js +++ b/core/server/models/settings.js @@ -124,7 +124,12 @@ Settings = ghostBookshelf.Model.extend({ async onValidate(model, attr, options) { await ghostBookshelf.Model.prototype.onValidate.call(this, model, attr, options); - await validation.validateSettings(getDefaultSettings(), model); + + await Settings.validators.all(model); + + if (typeof Settings.validators[model.get('key')] === 'function') { + await Settings.validators[model.get('key')](model); + } }, format() { @@ -325,6 +330,116 @@ Settings = ghostBookshelf.Model.extend({ return Promise.reject(new errors.NoPermissionError({ message: i18n.t('errors.models.post.notEnoughPermission') })); + }, + + validators: { + async all(model) { + const settingName = model.get('key'); + const settingDefault = getDefaultSettings()[settingName]; + + if (!settingDefault) { + return; + } + + // Basic validations from default-settings.json + const validationErrors = validation.validate( + model.get('value'), + model.get('key'), + settingDefault.validations, + 'settings' + ); + + if (validationErrors.length) { + throw new errors.ValidationError(validationErrors.join('\n')); + } + }, + async stripe_plans(model) { + const plans = JSON.parse(model.get('value')); + for (const plan of plans) { + // We check 100, not 1, because amounts are in fractional units + if (plan.amount < 100 && plan.name !== 'Complimentary') { + throw new errors.ValidationError({ + message: 'Plans cannot have an amount less than 1' + }); + } + + if (typeof plan.name !== 'string') { + throw new errors.ValidationError({ + message: 'Plan must have a name' + }); + } + + if (typeof plan.currency !== 'string') { + throw new errors.ValidationError({ + message: 'Plan must have a currency' + }); + } + + if (!['year', 'month', 'week', 'day'].includes(plan.interval)) { + throw new errors.ValidationError({ + message: 'Plan interval must be one of: year, month, week or day' + }); + } + } + }, + // @TODO: Maybe move some of the logic into the members service, exporting an isValidStripeKey + // method which can be called here, cleaning up the duplication, but not removing control + async stripe_secret_key(model) { + const value = model.get('value'); + if (value === null) { + return; + } + + const secretKeyRegex = /(?:sk|rk)_(?:test|live)_[\da-zA-Z]{1,247}$/; + + if (!secretKeyRegex.test(value)) { + throw new errors.ValidationError({ + message: `stripe_secret_key did not match ${secretKeyRegex}` + }); + } + }, + async stripe_publishable_key(model) { + const value = model.get('value'); + if (value === null) { + return; + } + + const secretKeyRegex = /pk_(?:test|live)_[\da-zA-Z]{1,247}$/; + + if (!secretKeyRegex.test(value)) { + throw new errors.ValidationError({ + message: `stripe_secret_key did not match ${secretKeyRegex}` + }); + } + }, + async stripe_connect_secret_key(model) { + const value = model.get('value'); + if (value === null) { + return; + } + + const secretKeyRegex = /(?:sk|rk)_(?:test|live)_[\da-zA-Z]{1,247}$/; + + if (!secretKeyRegex.test(value)) { + throw new errors.ValidationError({ + message: `stripe_secret_key did not match ${secretKeyRegex}` + }); + } + }, + async stripe_connect_publishable_key(model) { + const value = model.get('value'); + if (value === null) { + return; + } + + const secretKeyRegex = /pk_(?:test|live)_[\da-zA-Z]{1,247}$/; + + if (!secretKeyRegex.test(value)) { + throw new errors.ValidationError({ + message: `stripe_secret_key did not match ${secretKeyRegex}` + }); + } + } } }); diff --git a/test/unit/data/validation/index_spec.js b/test/unit/data/validation/index_spec.js index 10360776ac..553e6ee9b0 100644 --- a/test/unit/data/validation/index_spec.js +++ b/test/unit/data/validation/index_spec.js @@ -15,13 +15,12 @@ describe('Validation', function () { should.exist(validation); validation.should.have.properties( - ['validate', 'validator', 'validateSchema', 'validateSettings'] + ['validate', 'validator', 'validateSchema'] ); validation.validate.should.be.a.Function(); validation.validatePassword.should.be.a.Function(); validation.validateSchema.should.be.a.Function(); - validation.validateSettings.should.be.a.Function(); validation.validator.should.have.properties(['empty', 'notContains', 'isTimezone', 'isEmptyOrURL', 'isSlug']); }); diff --git a/test/unit/models/settings_spec.js b/test/unit/models/settings_spec.js index 1f6bca1245..b976721bce 100644 --- a/test/unit/models/settings_spec.js +++ b/test/unit/models/settings_spec.js @@ -5,6 +5,7 @@ const models = require('../../../core/server/models'); const {knex} = require('../../../core/server/data/db'); const {events} = require('../../../core/server/lib/common'); const defaultSettings = require('../../../core/server/data/schema/default-settings'); +const errors = require('@tryghost/errors'); describe('Unit: models/settings', function () { before(function () { @@ -211,4 +212,193 @@ describe('Unit: models/settings', function () { should.equal(returns.value, 'null'); }); }); + + describe('validation', function () { + async function testInvalidSetting({key, value, type, group}) { + const setting = models.Settings.forge({key, value, type, group}); + + let error; + try { + await setting.save(); + error = null; + } catch (err) { + error = err; + } finally { + should.exist(error, `Setting Model should throw when saving invalid ${key}`); + should.ok(error instanceof errors.ValidationError, 'Setting Model should throw ValidationError'); + } + } + + async function testValidSetting({key, value, type, group}) { + mockDb.mock(knex); + const tracker = mockDb.getTracker(); + tracker.install(); + + tracker.on('query', (query) => { + query.response(); + }); + + const setting = models.Settings.forge({key, value, type, group}); + + let error; + try { + await setting.save(); + error = null; + } catch (err) { + error = err; + } finally { + tracker.uninstall(); + mockDb.unmock(knex); + should.not.exist(error, `Setting Model should not throw when saving valid ${key}`); + } + } + + it('throws when stripe_secret_key is invalid', async function () { + await testInvalidSetting({ + key: 'stripe_secret_key', + value: 'INVALID STRIPE SECRET KEY', + type: 'string', + group: 'members' + }); + }); + + it('throws when stripe_publishable_key is invalid', async function () { + await testInvalidSetting({ + key: 'stripe_publishable_key', + value: 'INVALID STRIPE PUBLISHABLE KEY', + type: 'string', + group: 'members' + }); + }); + + it('does not throw when stripe_secret_key is valid', async function () { + await testValidSetting({ + key: 'stripe_secret_key', + value: 'rk_live_abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ', + type: 'string', + group: 'members' + }); + await testValidSetting({ + key: 'stripe_secret_key', + value: 'sk_live_abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ', + type: 'string', + group: 'members' + }); + }); + + it('does not throw when stripe_publishable_key is valid', async function () { + await testValidSetting({ + key: 'stripe_publishable_key', + value: 'pk_live_abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ', + type: 'string', + group: 'members' + }); + }); + + it('throws when stripe_connect_secret_key is invalid', async function () { + await testInvalidSetting({ + key: 'stripe_connect_secret_key', + value: 'INVALID STRIPE CONNECT SECRET KEY', + type: 'string', + group: 'members' + }); + }); + + it('throws when stripe_connect_publishable_key is invalid', async function () { + await testInvalidSetting({ + key: 'stripe_connect_publishable_key', + value: 'INVALID STRIPE CONNECT PUBLISHABLE KEY', + type: 'string', + group: 'members' + }); + }); + + it('does not throw when stripe_connect_secret_key is valid', async function () { + await testValidSetting({ + key: 'stripe_connect_secret_key', + value: 'sk_live_abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ', + type: 'string', + group: 'members' + }); + }); + + it('does not throw when stripe_connect_publishable_key is valid', async function () { + await testValidSetting({ + key: 'stripe_connect_publishable_key', + value: 'pk_live_abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ', + type: 'string', + group: 'members' + }); + }); + + it('throws when stripe_plans has invalid name', async function () { + await testInvalidSetting({ + key: 'stripe_plans', + value: JSON.stringify([{ + name: null, + amount: 500, + interval: 'month', + currency: 'usd' + }]), + type: 'string', + group: 'members' + }); + }); + + it('throws when stripe_plans has invalid amount', async function () { + await testInvalidSetting({ + key: 'stripe_plans', + value: JSON.stringify([{ + name: 'Monthly', + amount: 0, + interval: 'month', + currency: 'usd' + }]), + type: 'string', + group: 'members' + }); + }); + + it('throws when stripe_plans has invalid interval', async function () { + await testInvalidSetting({ + key: 'stripe_plans', + value: JSON.stringify([{ + name: 'Monthly', + amount: 500, + interval: 'monthly', // should be 'month' + currency: 'usd' + }]), + type: 'string', + group: 'members' + }); + }); + + it('throws when stripe_plans has invalid currency', async function () { + await testInvalidSetting({ + key: 'stripe_plans', + value: JSON.stringify([{ + name: 'Monthly', + amount: 500, + interval: 'month', + currency: null + }]), + type: 'string', + group: 'members' + }); + }); + + it('does not throw when stripe_plans is valid', async function () { + await testValidSetting({ + key: 'stripe_plans', + value: JSON.stringify([{ + name: 'Monthly', + amount: 500, + interval: 'month', + currency: 'usd' + }]), + type: 'string', + group: 'members' + }); + }); + }); });