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

@@ -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
 };

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}`
+                });
+            }
+        }
     }
 });
 

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']);
     });

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'
+            });
+        });
+    });
 });