Switch settings.edit to use an allow list

refs: https://github.com/TryGhost/Team/issues/1625 - Ensure that we maintain a list of exactly which settings can be edited - Bypass this for internal settings changes for now - TODO: use the settingsBreadService internally instead of the api directly
2024-11-23 11:55:01 +03:00 · 2022-05-16 13:23:26 +01:00 · 2022-05-16 13:23:26 +01:00 · 6d66fe9e22
commit 6d66fe9e22
parent 349cbdfc07
8 changed files with 201 additions and 88 deletions
--- a/core/server/api/canary/notifications.js
+++ b/core/server/api/canary/notifications.js
@ -1,5 +1,6 @@
 const {notifications} = require('../../services/notifications');
-const api = require('./index');
+const settingsService = require('../../services/settings/settings-service');
+const settingsBREADService = settingsService.getSettingsBREADServiceInstance();
 const internalContext = {context: {internal: true}};

 module.exports = {
@ -25,19 +26,17 @@ module.exports = {
            }
        },
        permissions: true,
-        query(frame) {
+        async query(frame) {
            const {allNotifications, notificationsToAdd} = notifications.add({
                notifications: frame.data.notifications
            });

            if (notificationsToAdd.length){
-                return api.settings.edit({
-                    settings: [{
+                return await settingsBREADService.edit([{
                    key: 'notifications',
                    // @NOTE: We always need to store all notifications!
                    value: allNotifications.concat(notificationsToAdd)
-                    }]
-                }, internalContext).then(() => {
+                }], internalContext).then(() => {
                    return notificationsToAdd;
                });
            }
@ -63,12 +62,10 @@ module.exports = {
                }
            });

-            return api.settings.edit({
-                settings: [{
+            await settingsBREADService.edit([{
                key: 'notifications',
                value: allNotifications
-                }]
-            }, internalContext).return();
+            }], internalContext);
        }
    },

@ -82,15 +79,13 @@ module.exports = {
        permissions: {
            method: 'destroy'
        },
-        query() {
+        async query() {
            const allNotifications = notifications.destroyAll();

-            return api.settings.edit({
-                settings: [{
+            await settingsBREADService.edit([{
                key: 'notifications',
                value: allNotifications
-                }]
-            }, internalContext).return();
+            }], internalContext);
        }
    }
 };
--- a/core/server/api/canary/settings.js
+++ b/core/server/api/canary/settings.js
@ -15,6 +15,20 @@ const messages = {

 };

+async function getStripeConnectData(frame) {
+    const stripeConnectIntegrationToken = frame.data.settings.find(setting => setting.key === 'stripe_connect_integration_token');
+
+    if (stripeConnectIntegrationToken && stripeConnectIntegrationToken.value) {
+        const getSessionProp = prop => frame.original.session[prop];
+
+        return await settingsBREADService.getStripeConnectData(
+            stripeConnectIntegrationToken,
+            getSessionProp,
+            membersService.stripeConnect.getStripeConnectTokenData
+        );
+    }
+}
+
 module.exports = {
    docName: 'settings',

@ -171,20 +185,17 @@ module.exports = {
            }
        },
        async query(frame) {
-            let stripeConnectData;
-            const stripeConnectIntegrationToken = frame.data.settings.find(setting => setting.key === 'stripe_connect_integration_token');
+            let stripeConnectData = await getStripeConnectData(frame);

-            if (stripeConnectIntegrationToken && stripeConnectIntegrationToken.value) {
-                const getSessionProp = prop => frame.original.session[prop];
+            let result = await settingsBREADService.edit(frame.data.settings, frame.options, stripeConnectData);

-                stripeConnectData = await settingsBREADService.getStripeConnectData(
-                    stripeConnectIntegrationToken,
-                    getSessionProp,
-                    membersService.stripeConnect.getStripeConnectTokenData
-                );
+            if (_.isEmpty(result)) {
+                this.headers.cacheInvalidate = false;
+            } else {
+                this.headers.cacheInvalidate = true;
            }

-            return await settingsBREADService.edit(frame.data.settings, frame.options, stripeConnectData);
+            return result;
        }
    },

--- a/core/server/api/canary/utils/index.js
+++ b/core/server/api/canary/utils/index.js
@ -26,9 +26,7 @@ module.exports = {
        return frame.apiType === 'content';
    },

-    // @TODO: Remove, not used.
-    isAdminAPIKey: (frame) => {
-        return frame.options.context && Object.keys(frame.options.context).length !== 0 && frame.options.context.api_key &&
-            frame.options.context.api_key.type === 'admin';
+    isInternal: (frame) => {
+        return frame.options.context && frame.options.context.internal;
    }
 };
--- a/core/server/api/canary/utils/serializers/input/settings.js
+++ b/core/server/api/canary/utils/serializers/input/settings.js
@ -1,10 +1,65 @@
 const _ = require('lodash');
 const url = require('./utils/url');
+const localUtils = require('../../index');
 const settingsCache = require('../../../../../../shared/settings-cache');
 const {WRITABLE_KEYS_ALLOWLIST} = require('../../../../../../shared/labs');

-const DEPRECATED_SETTINGS = [
-    'slack', 'bulk_email_settings'
+const EDITABLE_SETTINGS = [
+    'title',
+    'description',
+    'logo',
+    'cover_image',
+    'icon',
+    'locale',
+    'timezone',
+    'codeinjection_head',
+    'codeinjection_foot',
+    'facebook',
+    'twitter',
+    'navigation',
+    'secondary_navigation',
+    'meta_title',
+    'meta_description',
+    'og_image',
+    'og_title',
+    'og_description',
+    'twitter_image',
+    'twitter_title',
+    'twitter_description',
+    'is_private',
+    'password',
+    'default_content_visibility',
+    'default_content_visibility_tiers',
+    'members_signup_access',
+    'stripe_secret_key',
+    'stripe_publishable_key',
+    'stripe_connect_secret_key',
+    'stripe_connect_publishable_key',
+    'stripe_connect_account_id',
+    'stripe_connect_display_name',
+    'stripe_connect_livemode',
+    'portal_name',
+    'portal_button',
+    'portal_plans',
+    'portal_button_style',
+    'firstpromoter',
+    'firstpromoter_id',
+    'portal_button_icon',
+    'portal_button_signup_text',
+    'mailgun_api_key',
+    'mailgun_domain',
+    'mailgun_base_url',
+    'email_track_opens',
+    'amp',
+    'amp_gtag_id',
+    'slack_url',
+    'slack_username',
+    'unsplash',
+    'shared_views',
+    'accent_color',
+    'editor_default_email_recipients',
+    'editor_default_email_recipients_filter',
+    'labs'
 ];

 module.exports = {
@ -13,28 +68,19 @@ module.exports = {
        if (_.isString(frame.data)) {
            frame.data = {settings: [{key: frame.data, value: frame.options}]};
        }
+
        const settings = settingsCache.getAll();

-        // Ignore and drop all values with Read-only flag
+        if (!localUtils.isInternal(frame)) {
+            // Ignore and drop all values not in the EDITABLE_SETTINGS list unless this is an internal request
            frame.data.settings = frame.data.settings.filter((setting) => {
-            const settingFlagsStr = settings[setting.key] ? settings[setting.key].flags : '';
-            const settingFlagsArr = settingFlagsStr ? settingFlagsStr.split(',') : [];
-            return !settingFlagsArr.includes('RO');
+                return EDITABLE_SETTINGS.includes(setting.key);
            });
+        }

        frame.data.settings.forEach((setting) => {
-            // CASE: transform objects/arrays into string (we store stringified objects in the db)
-            // @TODO: This belongs into the model layer. We should stringify before saving and parse when fetching from db.
-            // @TODO: Fix when dropping v0.1
            const settingType = settings[setting.key] ? settings[setting.key].type : '';

-            //TODO: Needs to be removed once we get rid of all `object` type settings
-            // NOTE: this transformation is more related to the fact that internal API calls call
-            //       settings API with plain objects instead of stringified ones
-            if (_.isObject(setting.value)) {
-                setting.value = JSON.stringify(setting.value);
-            }
-
            // @TODO: handle these transformations in a centralized API place (these rules should apply for ALL resources)

            // CASE: Ensure we won't forward strings, otherwise model events or model interactions can fail
@ -47,6 +93,7 @@ module.exports = {
                setting.value = setting.value === 'true';
            }

+            // CASE: filter labs to allowlist
            if (setting.key === 'labs') {
                const inputLabsValue = JSON.parse(setting.value);
                const filteredLabsValue = {};
@ -62,15 +109,5 @@ module.exports = {

            setting = url.forSetting(setting);
        });
-
-        // Ignore all deprecated settings
-        frame.data.settings = frame.data.settings.filter((setting) => {
-            // NOTE: ignore old unsplash object notation
-            if (setting.key === 'unsplash' && _.isObject(setting.value)) {
-                return true;
-            }
-
-            return DEPRECATED_SETTINGS.includes(setting.key) === false;
-        });
    }
 };
--- a/core/server/models/settings.js
+++ b/core/server/models/settings.js
@ -216,6 +216,11 @@ Settings = ghostBookshelf.Model.extend({
                return Promise.reject(new errors.ValidationError({message: tpl(messages.valueCannotBeBlank)}));
            }

+            // Ensure that object keys are stringified
+            if (_.isObject(item.value)) {
+                item.value = JSON.stringify(item.value);
+            }
+
            item = self.filterData(item);

            return Settings.forge({key: item.key}).fetch(options).then(function then(setting) {
--- a/test/e2e-api/admin/snapshots/settings.test.js.snap
+++ b/test/e2e-api/admin/snapshots/settings.test.js.snap
@ -1375,6 +1375,37 @@ Object {
 }
 `;

+exports[`Settings API Edit cannot edit uneditable settings 1: [body] 1`] = `
+Object {
+  "meta": Object {},
+  "settings": Array [],
+}
+`;
+
+exports[`Settings API Edit cannot edit uneditable settings 1: [headers] 1`] = `
+Object {
+  "access-control-allow-origin": "http://127.0.0.1:2369",
+  "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0",
+  "content-length": "272",
+  "content-type": "application/json; charset=utf-8",
+  "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
+  "vary": "Origin, Accept-Encoding",
+  "x-powered-by": "Express",
+}
+`;
+
+exports[`Settings API Edit cannot edit uneditable settings 2: [headers] 1`] = `
+Object {
+  "access-control-allow-origin": "http://127.0.0.1:2369",
+  "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0",
+  "content-length": "25",
+  "content-type": "application/json; charset=utf-8",
+  "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/,
+  "vary": "Origin, Accept-Encoding",
+  "x-powered-by": "Express",
+}
+`;
+
 exports[`Settings API can do disconnectStripeConnectIntegration 1: [headers] 1`] = `
 Object {
  "access-control-allow-origin": "http://127.0.0.1:2369",
--- a/test/e2e-api/admin/settings.test.js
+++ b/test/e2e-api/admin/settings.test.js
@ -163,6 +163,18 @@ describe('Settings API', function () {
                    etag: anyEtag
                });
        });
+
+        it('cannot edit uneditable settings', async function () {
+            await agent.put('settings/')
+                .body({
+                    settings: [{key: 'email_verification_required', value: false}]
+                })
+                .expectStatus(200)
+                .matchBodySnapshot()
+                .matchHeaderSnapshot({
+                    etag: anyEtag
+                });
+        });
    });

    describe('stripe connect', function () {
--- a/test/regression/api/admin/settings.test.js
+++ b/test/regression/api/admin/settings.test.js
@ -40,23 +40,25 @@ describe('Settings API (canary)', function () {
                });
        });

-        it('Can\'t edit permalinks', function (done) {
+        it('Can\'t edit permalinks', async function () {
            const settingToChange = {
                settings: [{key: 'permalinks', value: '/:primary_author/:slug/'}]
            };

-            request.put(localUtils.API.getApiQuery('settings/'))
+            await request.put(localUtils.API.getApiQuery('settings/'))
                .set('Origin', config.get('url'))
                .send(settingToChange)
                .expect('Content-Type', /json/)
                .expect('Cache-Control', testUtils.cacheRules.private)
-                .expect(404)
-                .end(function (err, res) {
-                    if (err) {
-                        return done(err);
-                    }
+                .expect(200)
+                .expect(({body, headers}) => {
+                    // it didn't actually edit anything, but we don't error anymore
+                    body.should.eql({
+                        settings: [],
+                        meta: {}
+                    });

-                    done();
+                    should.not.exist(headers['x-cache-invalidate']);
                });
        });

@ -110,22 +112,15 @@ describe('Settings API (canary)', function () {
                        .send(jsonResponse)
                        .expect('Content-Type', /json/)
                        .expect('Cache-Control', testUtils.cacheRules.private)
-                        .expect(404)
-                        .then(function ({body, headers}) {
-                            jsonResponse = body;
+                        .expect(200)
+                        .expect(({body, headers}) => {
+                            // it didn't actually edit anything, but we don't error anymore
+                            body.should.eql({
+                                settings: [],
+                                meta: {}
+                            });
+
                            should.not.exist(headers['x-cache-invalidate']);
-                            should.exist(jsonResponse.errors);
-                            testUtils.API.checkResponseValue(jsonResponse.errors[0], [
-                                'message',
-                                'context',
-                                'type',
-                                'details',
-                                'property',
-                                'help',
-                                'code',
-                                'id',
-                                'ghostErrorCode'
-                            ]);
                        });
                });
        });
@ -313,7 +308,16 @@ describe('Settings API (canary)', function () {
                .send(settingsToChange)
                .expect('Content-Type', /json/)
                .expect('Cache-Control', testUtils.cacheRules.private)
-                .expect(403);
+                .expect(200)
+                .expect(({body, headers}) => {
+                    // it didn't actually edit anything, but we don't error anymore
+                    body.should.eql({
+                        settings: [],
+                        meta: {}
+                    });
+
+                    should.not.exist(headers['x-cache-invalidate']);
+                });
        });
    });

@ -438,4 +442,24 @@ describe('Settings API (canary)', function () {
                });
        });
    });
+
+    // @TODO swap this internally for using settingsbread and then remove
+    describe('edit via context internal', function () {
+        const api = require('../../../../core/server/api').endpoints;
+
+        before(async function () {
+            await localUtils.startGhost();
+        });
+
+        it('allows editing settings that cannot be edited via HTTP', async function () {
+            let jsonResponse = await api.settings.edit({
+                settings: [{key: 'email_verification_required', value: false}]
+            }, testUtils.context.internal);
+
+            jsonResponse.should.eql({
+                settings: [{key: 'email_verification_required', value: false}],
+                meta: {}
+            });
+        });
+    });
 });