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
-
+