Security: restore stored secrets only for the same widget type and destination

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. */

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

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

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. */

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);

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