From e29a62aadb67fa4e8b054d06c28119c035738ca4 Mon Sep 17 00:00:00 2001 From: Thibaut Patel Date: Fri, 16 Apr 2021 17:05:16 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20Added=20a=20way=20to=20hide=20th?= =?UTF-8?q?e=20secret=20settings=20once=20they=20are=20set?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit issue https://github.com/TryGhost/Team/issues/621 --- core/server/api/canary/settings.js | 12 +++- core/server/api/v2/settings.js | 11 +++- core/server/api/v3/settings.js | 12 +++- core/server/services/settings/index.js | 22 ++++++- .../api/canary/admin/settings_spec.js | 60 +++++++++++++++++++ 5 files changed, 108 insertions(+), 9 deletions(-) diff --git a/core/server/api/canary/settings.js b/core/server/api/canary/settings.js index 77eb21a7ac..9be2f352f0 100644 --- a/core/server/api/canary/settings.js +++ b/core/server/api/canary/settings.js @@ -26,12 +26,14 @@ module.exports = { })); } - // CASE: omit core settings unless internal request if (!frame.options.context.internal) { + // CASE: omit core settings unless internal request settings = _.filter(settings, (setting) => { const isCore = setting.group === 'core'; return !isCore; }); + // CASE: omit secret settings unless internal request + settings = settings.map(settingsService.hideValueIfSecret); } return settings; @@ -70,6 +72,8 @@ module.exports = { })); } + setting = settingsService.hideValueIfSecret(setting); + return { [frame.options.key]: setting }; @@ -217,8 +221,8 @@ module.exports = { async query(frame) { const stripeConnectIntegrationToken = frame.data.settings.find(setting => setting.key === 'stripe_connect_integration_token'); - // The `stripe_connect_integration_token` "setting" is only used to set the `stripe_connect_*` settings. const settings = frame.data.settings.filter((setting) => { + // The `stripe_connect_integration_token` "setting" is only used to set the `stripe_connect_*` settings. return ![ 'stripe_connect_integration_token', 'stripe_connect_publishable_key', @@ -226,7 +230,9 @@ module.exports = { 'stripe_connect_livemode', 'stripe_connect_account_id', 'stripe_connect_display_name' - ].includes(setting.key); + ].includes(setting.key) + // Remove obfuscated settings + && !(setting.value === settingsService.obfuscatedSetting && settingsService.isSecretSetting(setting)); }); const getSetting = setting => settingsCache.get(setting.key, {resolve: false}); diff --git a/core/server/api/v2/settings.js b/core/server/api/v2/settings.js index 06ce01c80e..0a9fcf6d5b 100644 --- a/core/server/api/v2/settings.js +++ b/core/server/api/v2/settings.js @@ -24,12 +24,14 @@ module.exports = { })); } - // CASE: omit core settings unless internal request if (!frame.options.context.internal) { + // CASE: omit core settings unless internal request settings = _.filter(settings, (setting) => { const isCore = setting.group === 'core'; return !isCore; }); + // CASE: omit secret settings unless internal request + settings = settings.map(settingsService.hideValueIfSecret); } return settings; @@ -68,6 +70,8 @@ module.exports = { })); } + setting = settingsService.hideValueIfSecret(setting); + return { [frame.options.key]: setting }; @@ -108,7 +112,10 @@ module.exports = { } frame.data.settings = _.reject(frame.data.settings, (setting) => { - return setting.key === 'type'; + return setting.key === 'type' + // Remove obfuscated settings + || (setting.value === settingsService.obfuscatedSetting + && settingsService.isSecretSetting(setting)); }); const errors = []; diff --git a/core/server/api/v3/settings.js b/core/server/api/v3/settings.js index 77eb21a7ac..9be2f352f0 100644 --- a/core/server/api/v3/settings.js +++ b/core/server/api/v3/settings.js @@ -26,12 +26,14 @@ module.exports = { })); } - // CASE: omit core settings unless internal request if (!frame.options.context.internal) { + // CASE: omit core settings unless internal request settings = _.filter(settings, (setting) => { const isCore = setting.group === 'core'; return !isCore; }); + // CASE: omit secret settings unless internal request + settings = settings.map(settingsService.hideValueIfSecret); } return settings; @@ -70,6 +72,8 @@ module.exports = { })); } + setting = settingsService.hideValueIfSecret(setting); + return { [frame.options.key]: setting }; @@ -217,8 +221,8 @@ module.exports = { async query(frame) { const stripeConnectIntegrationToken = frame.data.settings.find(setting => setting.key === 'stripe_connect_integration_token'); - // The `stripe_connect_integration_token` "setting" is only used to set the `stripe_connect_*` settings. const settings = frame.data.settings.filter((setting) => { + // The `stripe_connect_integration_token` "setting" is only used to set the `stripe_connect_*` settings. return ![ 'stripe_connect_integration_token', 'stripe_connect_publishable_key', @@ -226,7 +230,9 @@ module.exports = { 'stripe_connect_livemode', 'stripe_connect_account_id', 'stripe_connect_display_name' - ].includes(setting.key); + ].includes(setting.key) + // Remove obfuscated settings + && !(setting.value === settingsService.obfuscatedSetting && settingsService.isSecretSetting(setting)); }); const getSetting = setting => settingsCache.get(setting.key, {resolve: false}); diff --git a/core/server/services/settings/index.js b/core/server/services/settings/index.js index e4ef0b6551..7f94bb4fcd 100644 --- a/core/server/services/settings/index.js +++ b/core/server/services/settings/index.js @@ -5,6 +5,22 @@ const models = require('../../models'); const SettingsCache = require('./cache'); +// The string returned when a setting is set as write-only +const obfuscatedSetting = '••••••••'; + +// The function used to decide whether a setting is write-only +function isSecretSetting(setting) { + return /secret/.test(setting.key); +} + +// The function that obfuscates a write-only setting +function hideValueIfSecret(setting) { + if (setting.value && isSecretSetting(setting)) { + return {...setting, value: obfuscatedSetting}; + } + return setting; +} + module.exports = { async init() { const settingsCollection = await models.Settings.populateDefaults(); @@ -44,5 +60,9 @@ module.exports = { value: currentRoutesHash }], {context: {internal: true}}); } - } + }, + + obfuscatedSetting, + isSecretSetting, + hideValueIfSecret }; diff --git a/test/regression/api/canary/admin/settings_spec.js b/test/regression/api/canary/admin/settings_spec.js index 1f6141cf6d..d755bfeff5 100644 --- a/test/regression/api/canary/admin/settings_spec.js +++ b/test/regression/api/canary/admin/settings_spec.js @@ -225,6 +225,66 @@ describe('Settings API (canary)', function () { .expect(403); }); + it('Can\'t read secret setting', function (done) { + const key = 'stripe_secret_key'; + request + .get(localUtils.API.getApiQuery(`settings/${key}/`)) + .set('Origin', config.get('url')) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + const json = res.body; + should.exist(json); + should.exist(json.settings); + + json.settings.length.should.eql(1); + json.settings[0].key.should.eql('stripe_secret_key'); + should(json.settings[0].value).be.null(); + + done(); + }); + }); + + it('Can\'t read secret setting', function (done) { + const key = 'stripe_secret_key'; + request.put(localUtils.API.getApiQuery('settings/')) + .set('Origin', config.get('url')) + .send({ + settings: [{ + key, + value: 'sk_test_4eC39HqLyjWDarjtT1zdp7dc' + }] + }) + .then(() => { + request + .get(localUtils.API.getApiQuery(`settings/${key}/`)) + .set('Origin', config.get('url')) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + const json = res.body; + should.exist(json); + should.exist(json.settings); + + json.settings.length.should.eql(1); + json.settings[0].key.should.eql('stripe_secret_key'); + json.settings[0].value.should.eql('••••••••'); + + done(); + }); + }); + }); + it('Can\'t read permalinks', function (done) { request.get(localUtils.API.getApiQuery('settings/permalinks/')) .set('Origin', config.get('url'))