diff --git a/api/src/config-secrets.js b/api/src/config-secrets.js index 26e8f282..45cedf31 100644 --- a/api/src/config-secrets.js +++ b/api/src/config-secrets.js @@ -3,7 +3,12 @@ const { scrubConfigSecrets, preserveConfigSecrets, secretSpec } = require('./widget-secrets'); const { scrubItemBadgeSecrets, preserveItemBadgeSecrets, toRows } = require('./badge-headers'); -const { badgeRequestMatchesSaved, widgetConfigMatchesSaved, rewriteChanged } = require('./secret-scope'); +const { + badgeRequestMatchesSaved, + widgetConfigMatchesSaved, + rewriteChanged, + widgetTlsChanged, +} = require('./secret-scope'); const { getRegistry } = require('./widgets'); function scrubAllSecrets(cfg) { @@ -38,6 +43,9 @@ function widgetHoldsSecret(config, entry) { return false; } +const mayHoldSecret = (config, entry) => + entry ? widgetHoldsSecret(config, entry) : !!config && typeof config === 'object' && Object.keys(config).length > 0; + /** Refill the secrets a save arrived without, but only for the items whose request is still the one the secret was stored for. Matching by item id alone sends a stored credential to a destination the incoming config names. @@ -58,7 +66,7 @@ function preserveAllSecrets(newCfg, oldCfg) { if (!prev) continue; const holds = prev.type === 'widget' - ? widgetHoldsSecret(prev.widgetConfig, reg[prev.widgetType]) + ? mayHoldSecret(prev.widgetConfig, reg[prev.widgetType]) : blockHoldsSecret(prev.badge) || blockHoldsSecret(prev.monitoring?.activity); if (holds) note(item); } @@ -70,13 +78,19 @@ function preserveAllSecrets(newCfg, oldCfg) { const reg = getRegistry(); const scopedForWidgets = { ...newCfg, items: [] }; const keepWidget = new Map(); + const tlsChanged = widgetTlsChanged(newCfg.settings?.server, oldCfg?.settings?.server); for (const item of newCfg.items) { if (!item || item.type !== 'widget') continue; const prev = prevOf(item); + if (!prev) continue; const entry = reg[item.widgetType]; - const ok = !prev || !entry || widgetConfigMatchesSaved(item.widgetConfig || {}, prev.widgetConfig || {}, entry); + const ok = + prev.type === 'widget' && + prev.widgetType === item.widgetType && + !tlsChanged && + (!entry || widgetConfigMatchesSaved(item.widgetConfig || {}, prev.widgetConfig || {}, entry)); keepWidget.set(item, ok); - if (!ok && widgetHoldsSecret(prev.widgetConfig, entry)) note(item); + if (!ok && mayHoldSecret(prev.widgetConfig, reg[prev.widgetType])) note(item); } /* preserveConfigSecrets matches oldCfg.items by id. Hide an item from it to stop its secret being refilled. */ diff --git a/api/src/routes/config.js b/api/src/routes/config.js index 8b2e82b4..8f9d7fbc 100644 --- a/api/src/routes/config.js +++ b/api/src/routes/config.js @@ -190,11 +190,12 @@ on('POST', '/api/config', async (req, res) => { server.allowedHosts = list; } data.settings.server = server; - /* A stored credential is only refilled for the request it was stored for. */ + /* Migrate first. A migration that renames a widget type after the refill + stores secrets the new manifest does not scrub. */ + migrate(data); const { withheld } = preserveAllSecrets(data, existing); if (redirected && existing.settings?.background?.apiKey && !data.settings?.background?.apiKey) withheld.push({ id: 'background', label: 'Unsplash' }); - migrate(data); ensureSystemItems(data); saveConfig(data); /* Only now: until the config points at it, the file being replaced is still diff --git a/api/src/secret-scope.js b/api/src/secret-scope.js index d244a6fc..33659b70 100644 --- a/api/src/secret-scope.js +++ b/api/src/secret-scope.js @@ -89,10 +89,16 @@ function badgeRequestMatchesSaved(request, stored) { @param {any} next @param {any} prev @returns {boolean} */ function rewriteChanged(next, prev) { const map = s => (s && s.portMap && typeof s.portMap === 'object' ? s.portMap : {}); - if (!Object.keys(map(next)).length) return false; + if (!Object.keys(map(next)).length && !Object.keys(map(prev)).length) return false; return !stableEqual(map(next), map(prev)) || (next?.hostIp || '') !== (prev?.hostIp || ''); } +/** Whether a save turns the global certificate skip on or off. Widget requests + fall back to it. @param {any} next @param {any} prev @returns {boolean} */ +function widgetTlsChanged(next, prev) { + return (next?.skipTlsVerify === true) !== (prev?.skipTlsVerify === true); +} + const RETYPE_MESSAGE = 'This configuration has changed since it was saved, so the stored credential was not used. ' + 'Enter the credential to test these settings.'; @@ -105,5 +111,6 @@ module.exports = { rowsMatch, badgeRequestMatchesSaved, rewriteChanged, + widgetTlsChanged, RETYPE_MESSAGE, }; diff --git a/api/src/widget-data.js b/api/src/widget-data.js index c5d0cd89..0bdc4262 100644 --- a/api/src/widget-data.js +++ b/api/src/widget-data.js @@ -153,7 +153,9 @@ on('POST', '/api/widget-options/:id', async (req, res) => { widgetType: body.widgetType, widgetConfig: body.widgetConfig || {}, }; - const saved = (loadConfig().items || []).find(i => i.id === req.params.id && i.type === 'widget'); + const saved = (loadConfig().items || []).find( + i => i.id === req.params.id && i.type === 'widget' && i.widgetType === body.widgetType, + ); /* Only restore a blanked secret when the rest of the posted config matches what is saved. Otherwise the request picks the destination while the server supplies the credential. */ diff --git a/api/test/secret-rewrite.test.js b/api/test/secret-rewrite.test.js index 72dc0d77..4980b57b 100644 --- a/api/test/secret-rewrite.test.js +++ b/api/test/secret-rewrite.test.js @@ -123,9 +123,86 @@ test('turning off certificate checking on an app withholds its stored credential ); }); +async function post(cfg) { + const res = await fetch(`${base}/api/config`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', Origin: base }, + body: JSON.stringify(cfg), + }); + assert.equal(res.status, 200); + return res.json(); +} + +const MAP = { hostIp: '10.0.0.5', portMap: { 8989: { host: 'sonarr', port: '8989' } } }; + +for (const [name, serverSettings] of [ + ['removing', { hostIp: '10.0.0.5' }], + ['emptying', { hostIp: '10.0.0.5', portMap: {} }], +]) { + test(`${name} the port map withholds every stored credential`, async () => { + const cfg = loadConfig(); + saveConfig({ ...cfg, settings: { ...cfg.settings, server: MAP } }); + const sent = await fetch(`${base}/api/config`).then(r => r.json()); + sent.settings.server = serverSettings; + const body = await post(sent); + assert.deepEqual(body.withheld.map(w => w.id).sort(), ['app1', 'background', 'w1']); + assert.ok(!JSON.stringify(loadConfig()).includes(SECRET)); + }); +} + +test('turning on certificate skipping for widgets withholds stored widget credentials', async () => { + const body = await saveServer({ skipTlsVerify: true }); + assert.deepEqual( + body.withheld.map(w => w.id), + ['w1'], + ); + assert.equal(loadConfig().items.find(i => i.id === 'w1').widgetConfig.absKey, undefined); +}); + +test('a widget sent under a type a migration renames does not get its secret back', async () => { + const sent = await fetch(`${base}/api/config`).then(r => r.json()); + delete sent._schemaVersion; + const w1 = sent.items.find(i => i.id === 'w1'); + w1.widgetType = 'stats'; + w1.widgetConfig = {}; + const body = await post(sent); + assert.ok(!JSON.stringify(body).includes(SECRET)); + assert.ok(!JSON.stringify(await fetch(`${base}/api/config`).then(r => r.text())).includes(SECRET)); + assert.ok(body.withheld.some(w => w.id === 'w1')); +}); + +test('an old backup of the same widget keeps its secret through the migration', async () => { + const cfg = loadConfig(); + cfg.items.push({ + id: 'y1', + type: 'widget', + widgetType: 'system-summary', + widgetConfig: { statProvider: 'glances', glancesUrl: 'http://g.test', glancesPass: SECRET }, + }); + saveConfig(cfg); + const sent = await fetch(`${base}/api/config`).then(r => r.json()); + delete sent._schemaVersion; + const y1 = sent.items.find(i => i.id === 'y1'); + y1.widgetType = 'stats'; + y1.widgetConfig = { statProvider: 'glances', glancesUrl: 'http://g.test', widgetSubType: 'system-summary' }; + const body = await post(sent); + assert.ok(!body.withheld?.some(w => w.id === 'y1')); + assert.equal(loadConfig().items.find(i => i.id === 'y1').widgetConfig.glancesPass, SECRET); +}); + +test('a port map change names a widget whose config is withheld', async () => { + const cfg = loadConfig(); + cfg.items.push({ id: 'u1', type: 'widget', widgetType: 'no-such-widget', widgetConfig: { url: 'http://u.test' } }); + saveConfig(cfg); + const body = await saveServer({ hostIp: '10.0.0.5', portMap: { 8989: { host: 'other', port: '8989' } } }); + assert.ok(body.withheld.some(w => w.id === 'u1')); +}); + test('rewriteChanged', () => { const m = { 80: { host: 'a', port: '80' } }; assert.equal(rewriteChanged({ hostIp: 'x' }, {}), false); + assert.equal(rewriteChanged({ hostIp: 'x' }, { hostIp: 'x', portMap: m }), true); + assert.equal(rewriteChanged({ hostIp: 'x', portMap: {} }, { hostIp: 'x', portMap: m }), true); assert.equal(rewriteChanged({ hostIp: 'x', portMap: m }, { hostIp: 'x', portMap: m }), false); assert.equal(rewriteChanged({ hostIp: 'x', portMap: m }, { hostIp: 'x' }), true); assert.equal(rewriteChanged({ hostIp: 'y', portMap: m }, { hostIp: 'x', portMap: m }), true); diff --git a/api/test/secret-widget-type.test.js b/api/test/secret-widget-type.test.js new file mode 100644 index 00000000..3c711ecb --- /dev/null +++ b/api/test/secret-widget-type.test.js @@ -0,0 +1,96 @@ +const path = require('node:path'); +const fs = require('node:fs'); +const { tmpDir } = require('../test-support/tmp'); +const _tmp = tmpDir('secret-type'); +process.env.CONFIG_PATH = path.join(_tmp, 'apps.json'); +process.env.WIDGETS_PATH = path.join(_tmp, 'widgets'); + +/* Two widgets that declare the same secret key. The second one returns its key, + so a refill across types shows up in the response. */ +for (const [name, data] of [ + ['alpha', 'module.exports = async () => ({ options: [] });\n'], + ['beta', 'module.exports = async ctx => ({ options: [{ value: String(ctx.config.key) }] });\n'], +]) { + const dir = path.join(_tmp, 'widgets', name); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + path.join(dir, 'widget.json'), + JSON.stringify({ + name, + label: name, + sizes: ['medium'], + views: { main: { src: 'index.html' } }, + fields: [ + { key: 'url', type: 'text', label: 'URL' }, + { key: 'key', type: 'secret', label: 'Key' }, + ], + }), + ); + fs.writeFileSync(path.join(dir, 'index.html'), ''); + fs.writeFileSync(path.join(dir, 'data.js'), data); +} + +const { test, before, after, beforeEach } = require('node:test'); +const assert = require('node:assert/strict'); +const http = require('node:http'); + +require('../src/routes'); +require('../src/widget-data'); +const { dispatch } = require('../src/router'); +const { saveConfig } = require('../src/config'); +const { preserveAllSecrets } = require('../src/config-secrets'); + +const SECRET = 'ALPHA-SECRET'; +const stored = () => ({ + items: [{ id: 'w1', type: 'widget', widgetType: 'alpha', widgetConfig: { url: 'http://a.test', key: SECRET } }], +}); +let server, base; + +before(async () => { + server = http.createServer(dispatch); + await new Promise(r => server.listen(0, '127.0.0.1', r)); + base = `http://127.0.0.1:${server.address().port}`; +}); +after( + () => + new Promise(r => { + server.closeAllConnections?.(); + server.close(r); + }), +); +beforeEach(() => saveConfig(stored())); + +test('a save that changes the widget type does not refill its secret', () => { + const next = { items: [{ id: 'w1', type: 'widget', widgetType: 'beta', widgetConfig: { url: 'http://a.test' } }] }; + const { withheld } = preserveAllSecrets(next, stored()); + assert.equal(next.items[0].widgetConfig.key, undefined); + assert.deepEqual( + withheld.map(w => w.id), + ['w1'], + ); +}); + +test('a save that keeps the widget type still refills its secret', () => { + const next = { items: [{ id: 'w1', type: 'widget', widgetType: 'alpha', widgetConfig: { url: 'http://a.test' } }] }; + const { withheld } = preserveAllSecrets(next, stored()); + assert.equal(next.items[0].widgetConfig.key, SECRET); + assert.deepEqual(withheld, []); +}); + +const options = widgetType => + fetch(`${base}/api/widget-options/w1`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', Origin: base }, + body: JSON.stringify({ widgetType, endpoint: 'any', widgetConfig: { url: 'http://a.test' } }), + }).then(r => r.text()); + +test('an options fetch under the stored widget type gets the stored secret', async () => { + const cfg = stored(); + cfg.items[0].widgetType = 'beta'; + saveConfig(cfg); + assert.ok((await options('beta')).includes(SECRET)); +}); + +test('an options fetch under another widget type does not get the stored secret', async () => { + assert.ok(!(await options('beta')).includes(SECRET)); +});