From 534ac8117e12c0eb4c771b3537ea7d8c969f627b Mon Sep 17 00:00:00 2001 From: SandObserver <260779319+SandObserver@users.noreply.github.com> Date: Mon, 10 Aug 2026 23:18:47 -0400 Subject: [PATCH] Fix: stop a password being stored and deleted in one save Typing a password in the same save that switches Password Protection off stored it first, which rotates the session secret and signs every other device out, and the toggle then deleted it a moment later. The save reported success. The password is now left unwritten, and the field is cleared so it cannot read as one the dashboard holds. The toggle route accepts only a real true or false. An empty body, a null or a string used to be read as off, permanently deleting the stored password and secret over a request that said nothing. The sign-out row is resynced after protection is turned off, where it could stay on screen and answer with an error, and the change listener that kept it in step is attached again: it hung off an element that does not exist in the markup, so it never ran. The dead branch also wired the password strength meter, which admin.js already does. Password and Unsplash key status labels are translated rather than always reading in English. --- CHANGELOG.md | 17 +++++++- api/src/routes/auth.js | 16 +++++++- api/test/auth-lockout.test.js | 30 ++++++++++++++ ui/admin/index.html | 2 +- ui/i18n/de.json | 4 +- ui/i18n/en.json | 4 +- ui/i18n/es.json | 4 +- ui/i18n/fa.json | 4 +- ui/i18n/fr.json | 4 +- ui/i18n/zh-Hans.json | 4 +- ui/js/admin-logic.js | 8 ++++ ui/js/admin-settings.js | 73 ++++++++++++++++------------------- ui/js/admin.js | 4 +- ui/test/admin-logic.test.mjs | 23 +++++++++++ 14 files changed, 145 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd75a9c2..4428c521 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,11 +46,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Badges no longer lose their rounded shape in Safari after a long session. -- Turning Password Protection off now deletes the stored password, which - previously stayed behind and could leave an install unable to set a new one. +- Turning Password Protection off now deletes the stored password. + While protection is off, anyone who can reach the dashboard can set a new one. + +- A password typed in the same save that turns Password Protection off is no + longer stored and then immediately deleted, which signed out every other + device for nothing. + +- The sign-out row no longer stays on screen after Password Protection is + turned off, where pressing it returned an error. + +- The password and Unsplash key status labels are translated instead of always + reading in English. ### Security +- 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. + - A Connections tile that cannot reach a service now says so in plain words instead of printing the address it tried, which put an internal host and port on the dashboard. diff --git a/api/src/routes/auth.js b/api/src/routes/auth.js index 839b2cff..9ea03f28 100644 --- a/api/src/routes/auth.js +++ b/api/src/routes/auth.js @@ -160,6 +160,13 @@ on('POST', '/api/auth/toggle', async (req, res) => { if (!checkOrigin(req, res)) return; try { const { enabled } = JSON.parse(await readBody(req)); + /* Only a real true or false. Turning protection off deletes the password + and cannot be undone from the UI, so a body that does not say so plainly, + from an old cached page or a script, must change nothing rather than + being read as "off". */ + if (typeof enabled !== 'boolean') { + return json(res, 400, { error: 'enabled must be true or false', kind: KIND.INVALID }); + } const cfg = loadConfig(); cfg.settings = cfg.settings || {}; cfg.settings.auth = cfg.settings.auth || {}; @@ -176,7 +183,14 @@ on('POST', '/api/auth/toggle', async (req, res) => { can be obtained while auth is off, and login is a bypass that issues no cookie, so a forgotten password could only be replaced by editing the config on disk. The secret goes too, so tokens signed under it cannot be - honoured if protection is turned back on later. */ + honoured if protection is turned back on later. + + The trade this makes: with no hash stored, set-password no longer demands + a session, so anyone who can reach the dashboard while protection is off + can set one and lock the owner out. That is accepted rather than guarded, + because protection being off already means anyone who can reach the + dashboard can read and rewrite the whole config; a guard here would only + look like security. It is why the toggle asks before clearing. */ const cleared = !enabled && !!cfg.settings.auth.passwordHash; if (!enabled) { delete cfg.settings.auth.passwordHash; diff --git a/api/test/auth-lockout.test.js b/api/test/auth-lockout.test.js index fb997856..d3000c05 100644 --- a/api/test/auth-lockout.test.js +++ b/api/test/auth-lockout.test.js @@ -239,3 +239,33 @@ test('login still refuses a wrong password when one is set', async () => { assert.equal((await req('POST', '/api/auth/login', { password: 'nope' })).status, 401); assert.equal((await req('POST', '/api/auth/login', { password: 'correct-horse' })).status, 200); }); + +/* ── the switch only answers to a real true or false ──────────────────────── */ + +/* Switching off deletes the password and the secret, and nothing in the UI can + undo it. A body that does not plainly say so, from a cached older page or a + script, used to be read as "off" and destroyed both. */ + +for (const [label, body] of [ + ['an empty body', {}], + ['a null flag', { enabled: null }], + ['a zero', { enabled: 0 }], + ['a string', { enabled: 'false' }], + ['the string "true"', { enabled: 'true' }], + ['a missing key with other fields', { enable: false }], +]) { + test(`${label} changes nothing rather than clearing the password`, async () => { + const cfg = loadConfig(); + const hash = await hashPassword('correct-horse'); + cfg.settings.auth = { enabled: true, secret: SECRET, passwordHash: hash }; + saveConfig(cfg); + + const r = await req('POST', '/api/auth/toggle', body, 'ds=' + makeToken('s1', SECRET)); + assert.equal(r.status, 400, 'refused'); + + const stored = loadConfig().settings.auth; + assert.equal(stored.passwordHash, hash, 'the password survives'); + assert.equal(stored.secret, SECRET, 'the secret survives'); + assert.equal(stored.enabled, true, 'the flag is untouched'); + }); +} diff --git a/ui/admin/index.html b/ui/admin/index.html index 262bdf8b..7b9b0f15 100644 --- a/ui/admin/index.html +++ b/ui/admin/index.html @@ -285,6 +285,6 @@

General

- + diff --git a/ui/i18n/de.json b/ui/i18n/de.json index 88e885bc..1dde1d58 100644 --- a/ui/i18n/de.json +++ b/ui/i18n/de.json @@ -23,7 +23,9 @@ "edit": "Bearbeiten", "color": "Farbe", "create": "Erstellen", - "chooseFiles": "Dateien wählen" + "chooseFiles": "Dateien wählen", + "configured": "Eingerichtet", + "notSet": "Nicht gesetzt" }, "general": { "title": "Titel", diff --git a/ui/i18n/en.json b/ui/i18n/en.json index 3ff9b8b6..0564bd99 100644 --- a/ui/i18n/en.json +++ b/ui/i18n/en.json @@ -23,7 +23,9 @@ "edit": "Edit", "color": "Color", "create": "Create", - "chooseFiles": "Choose files" + "chooseFiles": "Choose files", + "configured": "Configured", + "notSet": "Not set" }, "general": { "title": "Title", diff --git a/ui/i18n/es.json b/ui/i18n/es.json index cc8388ae..da10b4bc 100644 --- a/ui/i18n/es.json +++ b/ui/i18n/es.json @@ -23,7 +23,9 @@ "edit": "Editar", "color": "Color", "create": "Crear", - "chooseFiles": "Elegir archivos" + "chooseFiles": "Elegir archivos", + "configured": "Configurado", + "notSet": "Sin definir" }, "general": { "title": "Título", diff --git a/ui/i18n/fa.json b/ui/i18n/fa.json index 630ea8ca..d9427fac 100644 --- a/ui/i18n/fa.json +++ b/ui/i18n/fa.json @@ -23,7 +23,9 @@ "edit": "ویرایش", "color": "رنگ", "create": "ساختن", - "chooseFiles": "انتخاب فایل‌ها" + "chooseFiles": "انتخاب فایل‌ها", + "configured": "تنظیم‌شده", + "notSet": "تنظیم نشده" }, "general": { "title": "عنوان", diff --git a/ui/i18n/fr.json b/ui/i18n/fr.json index 20493492..74571b0d 100644 --- a/ui/i18n/fr.json +++ b/ui/i18n/fr.json @@ -23,7 +23,9 @@ "edit": "Modifier", "color": "Couleur", "create": "Créer", - "chooseFiles": "Choisir des fichiers" + "chooseFiles": "Choisir des fichiers", + "configured": "Définie", + "notSet": "Non définie" }, "general": { "title": "Titre", diff --git a/ui/i18n/zh-Hans.json b/ui/i18n/zh-Hans.json index 4d6a2e14..bc1bcdec 100644 --- a/ui/i18n/zh-Hans.json +++ b/ui/i18n/zh-Hans.json @@ -23,7 +23,9 @@ "edit": "编辑", "color": "颜色", "create": "创建", - "chooseFiles": "选择文件" + "chooseFiles": "选择文件", + "configured": "已设置", + "notSet": "未设置" }, "general": { "title": "标题", diff --git a/ui/js/admin-logic.js b/ui/js/admin-logic.js index 64f827da..549154a2 100644 --- a/ui/js/admin-logic.js +++ b/ui/js/admin-logic.js @@ -118,6 +118,14 @@ export function authEnableBlocked({ enabled, passwordSet, newPassword }) { return !!enabled && !passwordSet && !(newPassword || '').length; } +/* A password typed in the same save that switches protection off is not stored. + Writing it would rotate the session secret and sign every other device out, + and the toggle in the same save would delete it again a moment later, so the + round trip costs the user their other sessions and leaves nothing behind. */ +export function shouldWritePassword({ enabled, newPassword }) { + return !!enabled && !!(newPassword || '').length; +} + /* 'registry' renders the manifest's fields, 'custom' the URL editor, and 'unavailable' a registry widget whose manifest is not loaded. The last must not fall through to the custom editor: the server withholds that widget's config, diff --git a/ui/js/admin-settings.js b/ui/js/admin-settings.js index 91ffaccd..53a2ed67 100644 --- a/ui/js/admin-settings.js +++ b/ui/js/admin-settings.js @@ -1,15 +1,29 @@ /* Admin UI: the General, Appearance and Security settings sections. */ import { toast, ag, ap } from '/js/admin-shared.js?v=182410cc'; -import { wirePasswordStrength } from '/js/admin-auth.js?v=dd849d4c'; import { pwStrength } from '/js/password-strength.js?v=dab9978e'; import { t } from '/js/i18n.js?v=133a7aac'; -import { authEnableBlocked } from '/js/admin-logic.js?v=056a11e9'; +import { authEnableBlocked, shouldWritePassword } from '/js/admin-logic.js?v=3c60f7b0'; import { el, inp, q, qa, setUserText } from '/js/utils.js?v=17424946'; /* Mirrors the server's rule: auth cannot be switched on with no password behind it. */ let _passwordSet = false; +/* Both controls only mean anything while auth is on; the revoke one also needs + a password to exist, since that is what makes a session possible. + + Module scope rather than a closure, because the three moments that change + what it reflects are in three different places: the check on load, the + toggle, and the save that clears a stored password. */ +function syncSessionRows() { + const on = !!inp('sec-en')?.checked; + el('sec-logout')?.classList.toggle('d-none', !on); + const canRevoke = on && _passwordSet; + el('sec-revoke-row')?.classList.toggle('d-none', !canRevoke); + const revokeTip = el('revoke-tip'); + if (revokeTip) revokeTip.style.display = canRevoke ? '' : 'none'; +} + export function loadSettings(c) { const s = c.settings || {}; const ld = inp('set-lbl-d'); @@ -73,9 +87,9 @@ export function loadSettings(c) { const vEl = el('ie-apikey-v'); if (!d.configured) { apiEl.placeholder = 'Paste your Unsplash API key'; - if (vEl) vEl.textContent = 'Not set'; + if (vEl) vEl.textContent = t('common.notSet'); } else { - if (vEl) vEl.textContent = 'Configured'; + if (vEl) vEl.textContent = t('common.configured'); } }) .catch(() => {}); @@ -127,7 +141,7 @@ export function loadSettings(c) { ); _sv('ie-ip-v', s.server?.hostIp, '192.168.1.100'); _sv('ie-socket-v', s.server?.socketProxyUrl, 'http://socket-proxy:2375'); - _sv('ie-pw-v', '', 'Not set'); /* set below after auth check */ + _sv('ie-pw-v', '', t('common.notSet')); /* set below after auth check */ const _si = (id, v) => { const node = inp(id); if (node && v != null) node.value = v; @@ -166,31 +180,8 @@ export function loadSettings(c) { el('srv-save').addEventListener('click', saveServer); const secEnEl = inp('sec-en'); - const secSubEl = el('sec-sub'); - const secPwEl = el('sec-pw'); - let pwStrengthWired = false; - function openSecSub() { - secSubEl.classList.add('open'); - /* Wire strength meter on first open, avoids Safari input event bug - where listeners on password fields in hidden containers don't fire */ - if (!pwStrengthWired && secPwEl) { - pwStrengthWired = true; - wirePasswordStrength('sec-pw', 'sec-pw-bars', 'sec-pw-hint'); - } - } const secLogout = el('sec-logout'); const secRevoke = inp('sec-revoke'); - const secRevokeRow = el('sec-revoke-row'); - const revokeTip = el('revoke-tip'); - /* Both controls only mean anything while auth is on; the revoke one also needs - a password to exist, since that is what makes a session possible. */ - const syncLogout = () => { - const on = !!secEnEl?.checked; - if (secLogout) secLogout.classList.toggle('d-none', !on); - const canRevoke = on && _passwordSet; - if (secRevokeRow) secRevokeRow.classList.toggle('d-none', !canRevoke); - if (revokeTip) revokeTip.style.display = canRevoke ? '' : 'none'; - }; secLogout?.addEventListener('click', async () => { await ap('/api/auth/logout', {}).catch(() => {}); location.reload(); @@ -209,13 +200,7 @@ export function loadSettings(c) { secRevoke.disabled = false; } }); - if (secEnEl && secSubEl) { - secEnEl.addEventListener('change', () => { - if (secEnEl.checked) openSecSub(); - else secSubEl.classList.remove('open'); - syncLogout(); - }); - } + secEnEl?.addEventListener('change', syncSessionRows); ag('/api/auth/check') .then(d => { @@ -230,8 +215,8 @@ export function loadSettings(c) { if (pwHint) pwHint.style.display = d.enabled ? '' : 'none'; } const pwValEl = el('ie-pw-v'); - if (pwValEl) pwValEl.textContent = d.passwordSet ? 'Configured' : 'Not set'; - syncLogout(); + if (pwValEl) pwValEl.textContent = d.passwordSet ? t('common.configured') : t('common.notSet'); + syncSessionRows(); }) .catch(() => {}); } @@ -316,7 +301,7 @@ async function saveServer() { toast(t('toast.authNeedsPassword'), 'err'); return; } - if (pw) { + if (shouldWritePassword({ enabled, newPassword: pw })) { const { ok, labelKey } = pwStrength(pw); if (!ok) { toast(t('toast.pwWeak', { label: t(labelKey) }), 'err'); @@ -336,9 +321,17 @@ async function saveServer() { claiming one is stored: re-enabling in this session needs a new one. */ _passwordSet = false; const pwValEl = el('ie-pw-v'); - if (pwValEl) pwValEl.textContent = 'Not set'; + if (pwValEl) pwValEl.textContent = t('common.notSet'); const pwEl = inp('sec-pw'); - if (pwEl) pwEl.placeholder = ''; + if (pwEl) { + pwEl.placeholder = ''; + /* Whatever was typed was not stored, so leaving it in the box would + read as a password this dashboard now has. */ + pwEl.value = ''; + } + /* The revoke row offers to end sessions that no longer exist, and the + server answers it with an error. */ + syncSessionRows(); } toast(t('toast.saved')); if (langChanged) location.reload(); diff --git a/ui/js/admin.js b/ui/js/admin.js index 97f25bf3..53d9ffc9 100644 --- a/ui/js/admin.js +++ b/ui/js/admin.js @@ -1,7 +1,7 @@ import { loadLocalIcons, resolveIcon, iconChain } from '/js/icons.js?v=a0ea3e4b'; import { clr as rc, sanitizeCssUrl, el, inp, q, qa, tgt, setUserText } from '/js/utils.js?v=17424946'; import { html, raw, setHtml } from '/js/html.js?v=ccec347c'; -import { reorderItems, resolveAdminSection } from '/js/admin-logic.js?v=056a11e9'; +import { reorderItems, resolveAdminSection } from '/js/admin-logic.js?v=3c60f7b0'; import { newItemId, buildAppItem, upsertItem, claimFolderChildren } from '/js/admin-save-logic.js?v=77cac1d1'; import { API, toast, ag, ap, initInlineEdit } from '/js/admin-shared.js?v=182410cc'; import { checkAuth, wirePasswordStrength } from '/js/admin-auth.js?v=dd849d4c'; @@ -9,7 +9,7 @@ import { state } from '/js/admin-state.js?v=3f9ad806'; import { buildWidgetForm } from '/js/admin-widget-form.js?v=653071f0'; import { buildAppForm, buildFolderForm, serializeKvRows } from '/js/admin-app-form.js?v=b46e5b19'; import { LANGUAGES, initI18n, t } from '/js/i18n.js?v=133a7aac'; -import { loadSettings, showBgFields } from '/js/admin-settings.js?v=c2d82934'; +import { loadSettings, showBgFields } from '/js/admin-settings.js?v=536793f6'; import { canJoinFolder, applyDrop } from '/js/admin-drag-logic.js?v=53aeaa55'; /* openModal is the item editor in this file, so the shared dialog comes in under a different name. */ diff --git a/ui/test/admin-logic.test.mjs b/ui/test/admin-logic.test.mjs index b712b0f5..ee04beae 100644 --- a/ui/test/admin-logic.test.mjs +++ b/ui/test/admin-logic.test.mjs @@ -8,6 +8,7 @@ import { visibleFieldKeys, clearsStoredSecret, authEnableBlocked, + shouldWritePassword, widgetConfigMode, resolveAdminSection, rejectionLines, @@ -370,3 +371,25 @@ test('the notice key follows the count', () => { assert.equal(refusedNoticeKey(2), 'widgetCfg.refusedPlural'); assert.equal(refusedNoticeKey(0), 'widgetCfg.refusedPlural'); }); + +/* ── shouldWritePassword ──────────────────────────────────────────────────── */ + +/* Typing a password and switching protection off in the same save is a + contradiction. Storing it first signed every other device out, because + set-password rotates the session secret, and the toggle in the same save then + deleted the password again. The round trip cost the user their sessions and + left nothing behind, and the save reported success. */ + +test('a password typed while switching protection off is not written', () => { + assert.equal(shouldWritePassword({ enabled: false, newPassword: 'correct-horse' }), false); +}); + +test('a password typed while switching protection on is written', () => { + assert.equal(shouldWritePassword({ enabled: true, newPassword: 'correct-horse' }), true); +}); + +test('nothing is written when no password was typed', () => { + assert.equal(shouldWritePassword({ enabled: true, newPassword: '' }), false); + assert.equal(shouldWritePassword({ enabled: true, newPassword: undefined }), false); + assert.equal(shouldWritePassword({ enabled: false, newPassword: '' }), false); +});