From 6d66fe9e2268a5a66ce6cb7dd913135f0dc1a213 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Mon, 16 May 2022 13:23:26 +0100 Subject: [PATCH] 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 --- core/server/api/canary/notifications.js | 39 ++++---- core/server/api/canary/settings.js | 31 +++++-- core/server/api/canary/utils/index.js | 6 +- .../utils/serializers/input/settings.js | 93 +++++++++++++------ core/server/models/settings.js | 5 + .../admin/__snapshots__/settings.test.js.snap | 31 +++++++ test/e2e-api/admin/settings.test.js | 12 +++ test/regression/api/admin/settings.test.js | 72 +++++++++----- 8 files changed, 201 insertions(+), 88 deletions(-) diff --git a/core/server/api/canary/notifications.js b/core/server/api/canary/notifications.js index 6a192471c3..4ba0328aa1 100644 --- 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: [{ - key: 'notifications', - // @NOTE: We always need to store all notifications! - value: allNotifications.concat(notificationsToAdd) - }] - }, internalContext).then(() => { + return await settingsBREADService.edit([{ + key: 'notifications', + // @NOTE: We always need to store all notifications! + value: allNotifications.concat(notificationsToAdd) + }], internalContext).then(() => { return notificationsToAdd; }); } @@ -63,12 +62,10 @@ module.exports = { } }); - return api.settings.edit({ - settings: [{ - key: 'notifications', - value: allNotifications - }] - }, internalContext).return(); + await settingsBREADService.edit([{ + key: 'notifications', + value: allNotifications + }], internalContext); } }, @@ -82,15 +79,13 @@ module.exports = { permissions: { method: 'destroy' }, - query() { + async query() { const allNotifications = notifications.destroyAll(); - return api.settings.edit({ - settings: [{ - key: 'notifications', - value: allNotifications - }] - }, internalContext).return(); + await settingsBREADService.edit([{ + key: 'notifications', + value: allNotifications + }], internalContext); } } }; diff --git a/core/server/api/canary/settings.js b/core/server/api/canary/settings.js index e50eddb20e..5c9b3ce1f3 100644 --- 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; } }, diff --git a/core/server/api/canary/utils/index.js b/core/server/api/canary/utils/index.js index 9ec0033b76..9f4078f4ef 100644 --- 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; } }; diff --git a/core/server/api/canary/utils/serializers/input/settings.js b/core/server/api/canary/utils/serializers/input/settings.js index a06f5093dc..5f4d496de4 100644 --- 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 - 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'); - }); + 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) => { + 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; - }); } }; diff --git a/core/server/models/settings.js b/core/server/models/settings.js index 8a57f39c7b..6bd445e269 100644 --- 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) { diff --git a/test/e2e-api/admin/__snapshots__/settings.test.js.snap b/test/e2e-api/admin/__snapshots__/settings.test.js.snap index 1b6f0cb56d..8f2cc1951b 100644 --- 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", diff --git a/test/e2e-api/admin/settings.test.js b/test/e2e-api/admin/settings.test.js index ff7ea8c675..eda6cd5a36 100644 --- 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 () { diff --git a/test/regression/api/admin/settings.test.js b/test/regression/api/admin/settings.test.js index a5c0383c14..424fcd31ff 100644 --- 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: {} + }); + }); + }); });