Fix: keep stored secrets per row when group rows change

Dockerfile

@@ -76,6 +76,7 @@ COPY ui/js/limits.js /app/ui/js/limits.js
 COPY ui/js/config-recovery-logic.js /app/ui/js/config-recovery-logic.js
 COPY ui/js/host-names.js /app/ui/js/host-names.js
 COPY ui/js/service-link.js /app/ui/js/service-link.js
+COPY ui/js/saved-row.js /app/ui/js/saved-row.js
 COPY scripts/exit-on-fatal.py /app/scripts/exit-on-fatal.py
 # Fails the build if python3 is no longer present. Without it the event listener
 # cannot start and no failure is reported.

api/src/config-secrets.js

@@ -3,12 +3,7 @@
 
 const { scrubConfigSecrets, preserveConfigSecrets, secretSpec } = require('./widget-secrets');
 const { scrubItemBadgeSecrets, preserveItemBadgeSecrets, toRows } = require('./badge-headers');
-const {
-  badgeRequestMatchesSaved,
-  widgetConfigMatchesSaved,
-  rewriteChanged,
-  widgetTlsChanged,
-} = require('./secret-scope');
+const { badgeRequestMatchesSaved, alignSavedConfig, rewriteChanged, widgetTlsChanged } = require('./secret-scope');
 const { getRegistry } = require('./widgets');
 
 function scrubAllSecrets(cfg) {
@@ -77,29 +72,34 @@ function preserveAllSecrets(newCfg, oldCfg) {
   if (Array.isArray(newCfg.items)) {
     const reg = getRegistry();
     const scopedForWidgets = { ...newCfg, items: [] };
+    /** @type {Map<any, any>} the saved item each widget may take secrets from, or null */
     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.type === 'widget' &&
-        prev.widgetType === item.widgetType &&
-        !tlsChanged &&
-        (!entry || widgetConfigMatchesSaved(item.widgetConfig || {}, prev.widgetConfig || {}, entry));
-      keepWidget.set(item, ok);
-      if (!ok && mayHoldSecret(prev.widgetConfig, reg[prev.widgetType])) note(item);
+      const sameWidget = prev.type === 'widget' && prev.widgetType === item.widgetType && !tlsChanged;
+      if (!entry) {
+        keepWidget.set(item, sameWidget ? prev : null);
+        if (!sameWidget && mayHoldSecret(prev.widgetConfig, reg[prev.widgetType])) note(item);
+        continue;
+      }
+      const aligned = sameWidget ? alignSavedConfig(item.widgetConfig || {}, prev.widgetConfig || {}, entry) : null;
+      keepWidget.set(item, aligned ? { ...prev, widgetConfig: aligned.config } : null);
+      if (aligned ? aligned.dropped : 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. */
     scopedForWidgets.items = newCfg.items;
     const oldForWidgets = {
       ...oldCfg,
-      items: oldItems.filter(o => {
+      items: oldItems.flatMap(o => {
         const match = newCfg.items.find(n => n && n.id === o.id && n.type === 'widget');
-        return !match || keepWidget.get(match) !== false;
+        if (!match || !keepWidget.has(match)) return [o];
+        const keep = keepWidget.get(match);
+        return keep ? [keep] : [];
       }),
     };
     preserveConfigSecrets(scopedForWidgets, oldForWidgets);

api/src/secret-scope.js

@@ -3,7 +3,7 @@
    id alone sends a stored credential anywhere the caller names. Fields the
    manifest marks cosmetic or transient are excluded. */
 
-const { secretSpec, cosmeticSpec, transientSpec } = require('./widget-secrets');
+const { SAVED_ROW, secretSpec, cosmeticSpec, transientSpec } = require('./widget-secrets');
 const { toRows } = require('./badge-headers');
 
 function stableEqual(a, b) {
@@ -38,31 +38,75 @@ function stripWidgetSecrets(config, entry) {
     }
     for (const [ok, subKeys] of Object.entries(spec.objects)) drop(out[ok], subKeys);
   }
+  for (const gk of Object.keys(secretSpec(entry).groups)) {
+    if (Array.isArray(out[gk])) for (const row of out[gk]) if (row && typeof row === 'object') delete row[SAVED_ROW];
+  }
   return out;
 }
 
+/* A row naming a saved row by SAVED_ROW gets that one, or none when the value is
+   not an index. Without it, a row matches by id, then by position. */
+function savedRowFor(row, i, oldRows) {
+  if (row && typeof row === 'object' && Object.hasOwn(row, SAVED_ROW)) {
+    const n = row[SAVED_ROW];
+    return Number.isInteger(n) && n >= 0 ? oldRows[n] : undefined;
+  }
+  return row && row.id != null ? oldRows.find(r => r && r.id === row.id) : oldRows[i];
+}
+
+/** The saved config with each secret-holding group lined up row for row with
+    the posted one, for preserveWidgetSecrets. A row gets its saved row only when
+    every field that can redirect the secret matches. Null when a field outside
+    those groups differs. `dropped` is true when a row left blank a stored secret
+    it does not get back.
+    @returns {{ config: Record<string, any>, dropped: boolean } | null} */
+function alignSavedConfig(newConfig, savedConfig, entry) {
+  if (!entry) return null;
+  const cfg = newConfig || {};
+  const saved = savedConfig || {};
+  const groups = secretSpec(entry).groups;
+  const shared = c => {
+    const out = { ...c };
+    for (const gk of Object.keys(groups)) delete out[gk];
+    return out;
+  };
+  if (!widgetConfigMatchesSaved(shared(cfg), shared(saved), entry)) return null;
+  const config = { ...saved };
+  let dropped = false;
+  for (const [gk, subKeys] of Object.entries(groups)) {
+    if (!Array.isArray(cfg[gk])) continue;
+    const oldRows = Array.isArray(saved[gk]) ? saved[gk] : [];
+    config[gk] = cfg[gk].map((row, i) => {
+      const old = savedRowFor(row, i, oldRows);
+      if (!old) return {};
+      if (widgetConfigMatchesSaved({ [gk]: [row] }, { [gk]: [old] }, entry)) return old;
+      if (subKeys.some(k => blankStored(row, old, k))) dropped = true;
+      return {};
+    });
+  }
+  return { config, dropped };
+}
+
 function widgetConfigMatchesSaved(newConfig, savedConfig, entry) {
   if (!entry) return false;
   return stableEqual(stripWidgetSecrets(newConfig, entry), stripWidgetSecrets(savedConfig, entry));
 }
 
+const blankStored = (obj, old, k) => !!old && old[k] != null && old[k] !== '' && !(obj && obj[k]);
+
 /** Whether the config leaves blank a secret the saved config holds. */
 function leavesStoredSecretBlank(config, savedConfig, entry) {
   const cfg = config || {};
   const saved = savedConfig || {};
-  const blank = (obj, old, k) => !!old && old[k] != null && old[k] !== '' && !(obj && obj[k]);
   const { topLevel, groups, objects } = secretSpec(entry);
-  if (topLevel.some(k => blank(cfg, saved, k))) return true;
+  if (topLevel.some(k => blankStored(cfg, saved, k))) return true;
   for (const [gk, subKeys] of Object.entries(groups)) {
     if (!Array.isArray(cfg[gk])) continue;
     const oldRows = Array.isArray(saved[gk]) ? saved[gk] : [];
-    const hit = cfg[gk].some((row, i) => {
-      const old = row && row.id != null ? oldRows.find(r => r && r.id === row.id) : oldRows[i];
-      return subKeys.some(k => blank(row, old, k));
-    });
+    const hit = cfg[gk].some((row, i) => subKeys.some(k => blankStored(row, savedRowFor(row, i, oldRows), k)));
     if (hit) return true;
   }
-  return Object.entries(objects).some(([ok, subKeys]) => subKeys.some(k => blank(cfg[ok], saved[ok], k)));
+  return Object.entries(objects).some(([ok, subKeys]) => subKeys.some(k => blankStored(cfg[ok], saved[ok], k)));
 }
 
 function rowsMatch(newRows, oldRows) {
@@ -107,6 +151,7 @@ module.exports = {
   stableEqual,
   stripWidgetSecrets,
   widgetConfigMatchesSaved,
+  alignSavedConfig,
   leavesStoredSecretBlank,
   rowsMatch,
   badgeRequestMatchesSaved,

api/src/widget-data.js

@@ -7,7 +7,7 @@ const { cpuSample, ramPercent, cpuTemp, diskStats, procCount, uptimeSeconds } =
 const { getRegistry, WIDGETS_PATH } = require('./widgets');
 const { translatorFor } = require('./widget-i18n');
 const { preserveWidgetSecrets } = require('./widget-secrets');
-const { widgetConfigMatchesSaved, leavesStoredSecretBlank, RETYPE_MESSAGE } = require('./secret-scope');
+const { alignSavedConfig, leavesStoredSecretBlank, RETYPE_MESSAGE } = require('./secret-scope');
 const { dispatchProvider } = require('./provider-dispatch');
 const { widgetSettings } = require('./widget-settings');
 const { IS_DEMO } = require('./demo');
@@ -215,9 +215,11 @@ on('POST', '/api/widget-options/:id', async (req, res) => {
   /* 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. */
-  const scoped = !!saved && widgetConfigMatchesSaved(item.widgetConfig, saved.widgetConfig, entry);
-  const withheld = !scoped && !!saved && leavesStoredSecretBlank(item.widgetConfig, saved.widgetConfig, entry);
-  if (scoped) preserveWidgetSecrets(item, saved, entry);
+  const aligned = saved ? alignSavedConfig(item.widgetConfig, saved.widgetConfig, entry) : null;
+  const withheld = aligned
+    ? aligned.dropped
+    : !!saved && leavesStoredSecretBlank(item.widgetConfig, saved.widgetConfig, entry);
+  if (aligned) preserveWidgetSecrets(item, { ...saved, widgetConfig: aligned.config }, entry);
 
   try {
     const out = await getWidgetData(

api/src/widget-secrets.js

@@ -1,4 +1,5 @@
 const { getRegistry } = require('./widgets');
+const { SAVED_ROW } = require('../../ui/js/saved-row.js');
 
 /* The matching field keys a widget declares, top level and one row deep. Test
    membership with Object.hasOwn. Config from disk inherits "constructor" and the
@@ -95,7 +96,8 @@ function scrubWidgetSecrets(item, entry) {
 }
 
 /* Mutates newItem.widgetConfig. Never store a "<key>Set" marker. A stored
-   marker outlives the secret it describes and can be asserted by the caller. */
+   marker outlives the secret it describes and can be asserted by the caller.
+   oldItem rows must line up with newItem rows: see alignSavedConfig. */
 function preserveWidgetSecrets(newItem, oldItem, entry) {
   const e = _entryFor(newItem, entry);
   if (!e || !newItem || !newItem.widgetConfig) return;
@@ -112,6 +114,7 @@ function preserveWidgetSecrets(newItem, oldItem, entry) {
     const oldRows = Array.isArray(owc[gk]) ? owc[gk] : [];
     nwc[gk].forEach((row, i) => {
       if (!row || typeof row !== 'object') return;
+      delete row[SAVED_ROW];
       /* Match by id first. Position alone misassigns a stored secret after a
          reorder or a delete. */
       const oldRow = (row.id != null ? oldRows.find(r => r && r.id === row.id) : oldRows[i]) || {};
@@ -180,6 +183,7 @@ function preserveConfigSecrets(newCfg, oldCfg) {
 }
 
 module.exports = {
+  SAVED_ROW,
   secretSpec,
   cosmeticSpec,
   transientSpec,

api/test/config-secrets.test.js

@@ -336,3 +336,54 @@ test('saving a weather display change keeps the stored OpenWeather key', () => {
   assert.deepEqual(withheld, []);
   assert.equal(sent.items[0].widgetConfig.owKey, 'OWK');
 });
+
+function backupWith(slots) {
+  return { items: [{ id: 'b1', type: 'widget', widgetType: 'backup', widgetConfig: { slots } }] };
+}
+
+const DUP_A = { provider: 'duplicati', dupUrl: 'http://a:8200', dupPass: 'PASS-A' };
+const DUP_B = { provider: 'duplicati', dupUrl: 'http://b:8200', dupPass: 'PASS-B' };
+const urlOnly = ({ dupPass, ...rest }) => rest;
+
+test('removing a row keeps the stored secret of every row that stays', () => {
+  const next = backupWith([{ ...urlOnly(DUP_B), savedRow: 1 }]);
+  const { withheld } = preserveAllSecrets(next, backupWith([DUP_A, DUP_B]));
+  assert.deepEqual(next.items[0].widgetConfig.slots, [DUP_B]);
+  assert.deepEqual(withheld, []);
+});
+
+test('adding a row keeps the stored secrets and gives the new row none', () => {
+  const added = { provider: 'duplicati', dupUrl: 'http://c:8200' };
+  const next = backupWith([
+    { ...urlOnly(DUP_A), savedRow: 0 },
+    { ...urlOnly(DUP_B), savedRow: 1 },
+    { ...added, savedRow: null },
+  ]);
+  const { withheld } = preserveAllSecrets(next, backupWith([DUP_A, DUP_B]));
+  assert.deepEqual(next.items[0].widgetConfig.slots, [DUP_A, DUP_B, added]);
+  assert.deepEqual(withheld, []);
+});
+
+test('a row whose address changed does not get its stored secret back', () => {
+  const moved = { provider: 'duplicati', dupUrl: 'http://evil:8200' };
+  const next = backupWith([
+    { ...moved, savedRow: 0 },
+    { ...urlOnly(DUP_B), savedRow: 1 },
+  ]);
+  const { withheld } = preserveAllSecrets(next, backupWith([DUP_A, DUP_B]));
+  assert.deepEqual(next.items[0].widgetConfig.slots, [moved, DUP_B]);
+  assert.deepEqual(withheld, [{ id: 'b1', label: 'b1' }]);
+});
+
+test('a row cannot take the secret of a saved row with another address', () => {
+  const next = backupWith([{ ...urlOnly(DUP_B), savedRow: 0 }]);
+  preserveAllSecrets(next, backupWith([DUP_A, DUP_B]));
+  assert.equal(next.items[0].widgetConfig.slots[0].dupPass, undefined);
+});
+
+test('without savedRow a removed row still withholds by position', () => {
+  const next = backupWith([urlOnly(DUP_B)]);
+  const { withheld } = preserveAllSecrets(next, backupWith([DUP_A, DUP_B]));
+  assert.equal(next.items[0].widgetConfig.slots[0].dupPass, undefined);
+  assert.deepEqual(withheld, [{ id: 'b1', label: 'b1' }]);
+});

api/test/secret-scope.test.js

@@ -11,6 +11,7 @@ const {
   rowsMatch,
   badgeRequestMatchesSaved,
   leavesStoredSecretBlank,
+  alignSavedConfig,
 } = require('../src/secret-scope');
 const { getRegistry } = require('../src/widgets');
 
@@ -244,3 +245,58 @@ test('a field that reaches the request still takes the stored credential out of
     assert.equal(widgetConfigMatchesSaved(changed, saved, r[name]), false, name);
   }
 });
+
+/* ── alignSavedConfig ─────────────────────────────────────────────────────── */
+
+const backup = getRegistry().backup;
+const SLOT_A = { provider: 'duplicati', dupUrl: 'http://a:8200', dupPass: 'PASS-A' };
+const SLOT_B = { provider: 'duplicati', dupUrl: 'http://b:8200', dupPass: 'PASS-B' };
+
+test('alignSavedConfig lines up each saved row with the row that names it', () => {
+  const out = alignSavedConfig(
+    { slots: [{ provider: 'duplicati', dupUrl: 'http://b:8200', dupPassSet: true, savedRow: 1 }] },
+    { slots: [SLOT_A, SLOT_B] },
+    backup,
+  );
+  assert.deepEqual(out, { config: { slots: [SLOT_B] }, dropped: false });
+});
+
+test('alignSavedConfig gives a changed row nothing and reports the dropped secret', () => {
+  const out = alignSavedConfig(
+    { slots: [{ provider: 'duplicati', dupUrl: 'http://evil:8200', savedRow: 0 }] },
+    { slots: [SLOT_A, SLOT_B] },
+    backup,
+  );
+  assert.deepEqual(out, { config: { slots: [{}] }, dropped: true });
+});
+
+test('alignSavedConfig ignores a savedRow that is not an index', () => {
+  for (const savedRow of [null, -1, 0.5, '0', '__proto__']) {
+    const out = alignSavedConfig({ slots: [{ ...SLOT_A, dupPass: undefined, savedRow }] }, { slots: [SLOT_A] }, backup);
+    assert.deepEqual(out.config.slots, [{}], String(savedRow));
+  }
+});
+
+test('alignSavedConfig refuses when a field outside the rows changed', () => {
+  assert.equal(alignSavedConfig({ absUrl: 'https://evil.example' }, SAVED, books), null);
+});
+
+test('stripWidgetSecrets drops savedRow, so an unchanged editor save still matches', () => {
+  assert.equal(
+    widgetConfigMatchesSaved(
+      { slots: [{ provider: 'duplicati', dupUrl: 'http://a:8200', savedRow: 0 }] },
+      { slots: [SLOT_A] },
+      backup,
+    ),
+    true,
+  );
+});
+
+test('leavesStoredSecretBlank follows savedRow', () => {
+  const rows = [{ provider: 'duplicati', dupUrl: 'http://b:8200', dupPass: 'typed', savedRow: 1 }];
+  assert.equal(leavesStoredSecretBlank({ slots: rows }, { slots: [SLOT_A, SLOT_B] }, backup), false);
+  assert.equal(
+    leavesStoredSecretBlank({ slots: [{ ...rows[0], dupPass: '' }] }, { slots: [SLOT_A, SLOT_B] }, backup),
+    true,
+  );
+});

e2e/admin.spec.js

@@ -154,6 +154,38 @@ test('a widget text field and secret field accept typing and save', async ({ pag
   expect(saved.widgetConfig.dnsPass ?? saved.widgetConfig.dnsPassSet).toBeTruthy();
 });
 
+test('removing a group row keeps the stored secret of the row that stays', async ({ page, request }) => {
+  const service = (name, url, token) => ({ type: 'netbird', name, url, token });
+  await seedConfig(request, {
+    items: [
+      {
+        id: 'net',
+        type: 'widget',
+        widgetType: 'connections',
+        label: 'Net',
+        widgetSize: 'medium',
+        widgetConfig: {
+          services: [service('A', 'http://a.invalid', 'token-a'), service('B', 'http://b.invalid', 'token-b')],
+        },
+      },
+    ],
+  });
+  await openDashboardList(page);
+  await rowByName(page, 'Net').getByRole('button', { name: /^Edit/ }).first().click();
+  await page.locator('#ev-body .grp-hdr-rm').first().click();
+
+  const tokenRow = page.locator('#ev-body .ie-row').filter({ has: page.locator('.rl', { hasText: /^Access token/ }) });
+  await expect(tokenRow).toHaveCount(1);
+  await expect(tokenRow.locator('.rv')).toHaveText('Configured');
+
+  const response = await saveEditor(page);
+  expect((await response.json()).withheld).toBeUndefined();
+  const saved = expectItem(await readConfig(request), i => i.id === 'net', 'the Connections widget');
+  expect(saved.widgetConfig.services).toHaveLength(1);
+  expect(saved.widgetConfig.services[0]).toMatchObject({ name: 'B', url: 'http://b.invalid', tokenSet: true });
+  expect(saved.widgetConfig.services[0]).not.toHaveProperty('savedRow');
+});
+
 test("a saved widget's Fetch sends its id and shows the server's advice", async ({ page, request }) => {
   await seedConfig(request, {
     items: [

tsconfig.frontend.json

@@ -278,6 +278,12 @@
       "/js/limits.js?v=*": [
         "./ui/js/limits.js"
       ],
+      "/js/saved-row.js": [
+        "./ui/js/saved-row.js"
+      ],
+      "/js/saved-row.js?v=*": [
+        "./ui/js/saved-row.js"
+      ],
       "/js/link-url.js": [
         "./ui/js/link-url.js"
       ],