From 5eb4f33d26bba54db9177fcd9b2edf7f7ac1e752 Mon Sep 17 00:00:00 2001 From: SandObserver <260779319+SandObserver@users.noreply.github.com> Date: Tue, 11 Aug 2026 21:16:37 -0400 Subject: [PATCH 1/3] Security: scope certificate skipping and shorten sessions Turning certificate verification off is now decided in one place and only ever applies to a host on the local network. The server-wide setting was already scoped that way; the per-app switch and the per-request flag were handed straight to the transport, and those are the paths that carry a stored credential. Session tokens now expire after 12 hours idle rather than 30 days, and a session still in use is reissued once it is past halfway, so the shorter window bounds an unused token without signing anyone out mid-task. --- CHANGELOG.md | 19 +++++ api/src/auth.js | 51 +++++++++++- api/src/proxy.js | 63 ++++++++++++++- api/src/router.js | 30 +++++-- api/src/routes/auth.js | 20 +++-- api/src/server.js | 9 ++- api/test/environment-doc.test.js | 4 +- api/test/http-integration.test.js | 67 +++++++++++++++- api/test/security-doc.test.js | 11 ++- api/test/session-renewal.test.js | 128 ++++++++++++++++++++++++++++++ api/test/skip-tls-scope.test.js | 105 ++++++++++++++++++++++++ docker-compose.yml | 5 +- docs/architecture.md | 2 +- docs/security.md | 10 ++- ui/admin/index.html | 4 +- ui/css/admin.css | 2 + ui/js/admin.js | 35 +++++++- ui/js/import-foreign.js | 18 +++++ ui/test/import-foreign.test.mjs | 49 ++++++++++++ 19 files changed, 594 insertions(+), 38 deletions(-) create mode 100644 api/test/session-renewal.test.js create mode 100644 api/test/skip-tls-scope.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index c3e00823..fa76afab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,6 +89,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - A failed connection test no longer writes the request path to the log, where a service that carries its API key in the path would have left it. +- Allowing a self-signed certificate now applies only to addresses on your own + network, wherever the request came from. + A badge or health check pointed at a public address is verified again, so an + app relying on this for one must use a trusted certificate. + +- Importing from another dashboard asks before allowing self-signed + certificates, instead of taking the setting from the file. + +- Sessions now expire after 12 hours idle instead of 30 days, and a session in + use is extended. Override with `SESSION_MAX_AGE_DAYS`. + +- Requests that change something must state their origin. + A script calling the API directly now has to send an `Origin` header naming + the address it is calling. + +- The API listens on loopback only, so it can no longer be reached around Nginx. + +- The pre-login status reply no longer says whether a password has been set. + ## [1.5.2] - 2026-08-09 ### Changed diff --git a/api/src/auth.js b/api/src/auth.js index e93059d5..ebbe332f 100644 --- a/api/src/auth.js +++ b/api/src/auth.js @@ -182,9 +182,21 @@ function needsRehash(hash) { } /* The signed issued-at inside the token is the real control; the cookie Max-Age - is only a browser hint kept in sync with it. */ + is only a browser hint kept in sync with it. + + This is an idle window, not a fixed one: a session in use is reissued by + refreshSession below, so the age here is how long a session survives with + nobody using it. A token that leaks is therefore usable for hours rather than + the month it used to be, while somebody working in the dashboard is not + signed out mid-task. */ +const DEFAULT_MAX_AGE_HOURS = 12; const _maxAgeDays = Number(process.env.SESSION_MAX_AGE_DAYS); -const SESSION_MAX_AGE_MS = (_maxAgeDays > 0 ? _maxAgeDays : 30) * 24 * 60 * 60 * 1000; +const SESSION_MAX_AGE_MS = _maxAgeDays > 0 ? _maxAgeDays * 24 * 60 * 60 * 1000 : DEFAULT_MAX_AGE_HOURS * 60 * 60 * 1000; + +/* Reissue once the token is past halfway. Renewing on every request would set a + cookie on every response for no gain; waiting until it nearly expires would + leave a tab open across the boundary holding a token it cannot renew. */ +const RENEW_AFTER_MS = SESSION_MAX_AGE_MS / 2; /* `${sessionId}.${issuedAt}.${sig}`, where sig covers the first two. Signing the timestamp is what lets verifyToken enforce a max age with no session store. */ @@ -195,7 +207,12 @@ function makeToken(sessionId, secret) { return `${payload}.${sig}`; } -function verifyToken(token, secret) { +/** The session a token stands for, with the moment it was issued, or null when + it does not verify or has aged out. Callers that only need the identity use + verifyToken; the issued-at is what refreshSession needs to decide on renewal. + @param {string} token @param {string} secret + @returns {{ sessionId: string, iat: number }|null} */ +function readToken(token, secret) { const dot2 = token.lastIndexOf('.'); if (dot2 === -1) return null; const sig = token.slice(dot2 + 1); @@ -209,7 +226,11 @@ function verifyToken(token, secret) { const expected = crypto.createHmac('sha256', secret).update(rest).digest('hex'); if (!crypto.timingSafeEqual(Buffer.from(sig, 'hex'), Buffer.from(expected, 'hex'))) return null; if (Date.now() - Number(iat) > SESSION_MAX_AGE_MS) return null; - return sessionId; + return { sessionId, iat: Number(iat) }; +} + +function verifyToken(token, secret) { + return readToken(token, secret)?.sessionId ?? null; } /* decodeURIComponent throws on an invalid escape, so a stray '%' in any cookie @@ -337,6 +358,25 @@ function isAuthenticated(req) { return !!verifyToken(token, secret); } +/* Extend a session that is still in use, so the shortened idle window does not + sign out somebody working in the dashboard. The identifier is carried over and + only the issued-at moves, so this lengthens a session rather than starting one. + + A handler that sets its own cookie runs after this and replaces the header, so + logging out and rotating the secret both still win. */ +function refreshSession(req, res) { + const cfg = loadConfig(); + if (!authActive(cfg)) return false; + const secret = cfg.settings.auth.secret; + if (!secret) return false; + const token = parseCookies(req).ds; + if (!token) return false; + const read = readToken(token, secret); + if (!read || Date.now() - read.iat < RENEW_AFTER_MS) return false; + setSessionCookie(res, makeToken(read.sessionId, secret), isSecureRequest(req)); + return true; +} + /* Unlike isAuthenticated, requires a real session even when auth is off. For operations such as changing an existing password. */ function hasValidSession(req) { @@ -362,6 +402,8 @@ module.exports = { parseHash, makeToken, verifyToken, + readToken, + refreshSession, parseCookies, setSessionCookie, clearSessionCookie, @@ -374,4 +416,5 @@ module.exports = { /* Tests exercise limits in sequence in one process. */ _resetRateLimits: () => _rateBuckets.clear(), SESSION_MAX_AGE_MS, + RENEW_AFTER_MS, }; diff --git a/api/src/proxy.js b/api/src/proxy.js index 3791f3d9..8cf5a8ed 100644 --- a/api/src/proxy.js +++ b/api/src/proxy.js @@ -122,6 +122,54 @@ function shouldSkipTls(hostname, cfg) { return isInternalHost(hostname); } +/* Whether to turn certificate checking off for this one request. + + The answer is the same wherever the request came from: only a host on your own + network. A self-signed certificate is a thing you have on a box you run, so + the switch that tolerates one has no honest use against a public address, + where the only effect is to accept whoever answers. The server-wide setting + was already scoped this way; the per-app switch and the per-request flag were + not, and those are the paths that carry a stored credential. + + `ignored` says the caller asked and was refused, so a certificate failure can + be explained rather than just reported. + + @param {string} hostname @param {boolean|null|undefined} requested + @returns {{ skip: boolean, ignored: boolean }} */ +function resolveSkipTls(hostname, requested) { + let wanted; + if (requested != null) wanted = requested === true; + else { + try { + wanted = loadConfig().settings?.server?.skipTlsVerify === true; + } catch { + wanted = false; + } + } + if (!wanted) return { skip: false, ignored: false }; + const internal = isInternalHost(hostname); + return { skip: internal, ignored: !internal }; +} + +/* Shown when a certificate failed on a host the skip could not apply to, so the + answer names the reason rather than looking like the switch did nothing. + + Wording only: it carries no hostname, path or upstream text, which is what + makes it safe to vouch for and show. See api-error.js. */ +const SKIP_TLS_IGNORED_MESSAGE = + 'The certificate could not be verified. Allowing a self-signed certificate only applies to addresses on your own ' + + 'network, so it was not used here.'; + +const TLS_ERROR_CODES = new Set([ + 'DEPTH_ZERO_SELF_SIGNED_CERT', + 'SELF_SIGNED_CERT_IN_CHAIN', + 'UNABLE_TO_VERIFY_LEAF_SIGNATURE', + 'UNABLE_TO_GET_ISSUER_CERT_LOCALLY', + 'CERT_HAS_EXPIRED', + 'CERT_NOT_YET_VALID', + 'ERR_TLS_CERT_ALTNAME_INVALID', +]); + function rewriteUrl(raw) { try { const cfg = loadConfig(), @@ -229,7 +277,7 @@ function fetchJSON(raw, opts = {}) { const done = (fn, arg) => dl.settle(fn, arg); const lib = u.protocol === 'https:' ? https : http; const port = u.port || (u.protocol === 'https:' ? 443 : 80); - const skipTls = opts.skipTls != null ? opts.skipTls : shouldSkipTls(u.hostname, loadConfig()); + const { skip: skipTls, ignored: skipIgnored } = resolveSkipTls(u.hostname, opts.skipTls); const bodyBuf = opts.body ? Buffer.from(opts.body) : null; const hdrs = Object.assign({}, opts.headers || {}); if (bodyBuf) hdrs['Content-Length'] = bodyBuf.length; @@ -299,7 +347,11 @@ function fetchJSON(raw, opts = {}) { req.destroy(); done(reject, new Error('Timed out')); }); - req.on('error', e => done(reject, e)); + req.on('error', (/** @type {unknown} */ e) => { + if (skipIgnored && TLS_ERROR_CODES.has(errCode(e) ?? '')) + /** @type {{ vouchedMessage?: string }} */ (e).vouchedMessage = SKIP_TLS_IGNORED_MESSAGE; + done(reject, e); + }); if (bodyBuf) req.write(bodyBuf); req.end(); }); @@ -356,7 +408,7 @@ function pingUrl(raw, ms = PING_MS, skipTls, pinIp) { if (policy) return resolve({ ok: false, status: 0, error: policy }); const lib = u.protocol === 'https:' ? https : http; const port = u.port || (u.protocol === 'https:' ? 443 : 80); - const skip = skipTls != null ? skipTls : shouldSkipTls(u.hostname, loadConfig()); + const { skip, ignored: skipIgnored } = resolveSkipTls(u.hostname, skipTls); const pin = pinIp && pinIp !== u.hostname ? pinIp : null; const connectHost = pin || u.hostname.replace(/^\[|\]$/g, ''); const opts = { hostname: connectHost, port, path: u.pathname || '/', timeout: ms, rejectUnauthorized: !skip }; @@ -392,7 +444,8 @@ function pingUrl(raw, ms = PING_MS, skipTls, pinIp) { operator's question is which service is unreachable, which the origin answers on its own. */ log.warn('ping failed', { url: u.origin, error: errMessage(e) }); - dl.settle(resolve, { ok: false, status: 0, error: pingErrorText(e), code: errCode(e) }); + const text = skipIgnored && TLS_ERROR_CODES.has(errCode(e) ?? '') ? SKIP_TLS_IGNORED_MESSAGE : pingErrorText(e); + dl.settle(resolve, { ok: false, status: 0, error: text, code: errCode(e) }); }); req.end(); }; @@ -460,6 +513,8 @@ module.exports = { rewriteUrl, getHostIp, shouldSkipTls, + resolveSkipTls, + SKIP_TLS_IGNORED_MESSAGE, isInternalHost, isDockerServiceName, bareHost, diff --git a/api/src/router.js b/api/src/router.js index f75a713f..8f05d50c 100644 --- a/api/src/router.js +++ b/api/src/router.js @@ -1,4 +1,4 @@ -const { isAuthenticated } = require('./auth'); +const { isAuthenticated, refreshSession } = require('./auth'); const log = require('./log'); const { tryDecode } = require('./percent-decode'); @@ -36,6 +36,9 @@ function route(req, res) { if (!PUBLIC_PATHS.has(u.pathname)) { if (!isAuthenticated(req)) return json(res, 401, { error: 'Unauthorised', auth: true, kind: 'auth' }); + /* Before the handler, so a handler that issues its own cookie replaces this + one rather than being overwritten by it. */ + refreshSession(req, res); } for (const r of routes) { @@ -116,15 +119,26 @@ function getIp(req) { } return peer || 'unknown'; } +/* Every browser sends Origin on a request that changes something, including a + same-origin one, so a write arriving without it did not come from a page on + this dashboard. It used to be allowed through, which left the check answering + only the requests that were already declaring themselves. + + This is what a command-line client has to add to write to the API: an Origin + naming the address it is calling. */ function checkOrigin(req, res) { const origin = req.headers['origin']; - if (!origin) return true; - try { - const originHost = new URL(origin).host; - const serverHost = req.headers['host']; - if (originHost === serverHost) return true; - } catch {} - json(res, 403, { error: 'Forbidden: origin mismatch', kind: 'invalid' }); + if (origin) { + try { + if (new URL(origin).host === req.headers['host']) return true; + } catch { + /* not a URL, so it names no host and cannot match one */ + } + } + json(res, 403, { + error: origin ? 'Forbidden: origin mismatch' : 'Forbidden: this request needs an Origin header', + kind: 'invalid', + }); return false; } diff --git a/api/src/routes/auth.js b/api/src/routes/auth.js index 9ea03f28..a3834c43 100644 --- a/api/src/routes/auth.js +++ b/api/src/routes/auth.js @@ -22,17 +22,27 @@ const { needsRehash, } = require('../auth'); +/* The one route that answers before sign-in, so it says only what the login + screen has to decide: whether to ask for a password, and whether this caller + already has a session. The two setup fields describe how the install is + configured and are added only once the caller is through, which is every case + that reads them: the setup prompt runs with protection off, where isAuthenticated + is true for everyone. */ on('GET', '/api/auth/check', (req, res) => { const cfg = loadConfig(); - json(res, 200, { + const authenticated = isAuthenticated(req); + const body = { /* The effective state, not the stored flag. Auth on with no password is reported as off because that is how it behaves; showing it as on would put a toggle in the admin UI that does not match what the server does. */ enabled: authActive(cfg), - authenticated: isAuthenticated(req), - passwordSet: !!cfg.settings?.auth?.passwordHash, - setupPrompted: !!cfg.settings?.auth?.setupPrompted, - }); + authenticated, + }; + if (authenticated) { + body.passwordSet = !!cfg.settings?.auth?.passwordHash; + body.setupPrompted = !!cfg.settings?.auth?.setupPrompted; + } + json(res, 200, body); }); on('POST', '/api/auth/login', async (req, res) => { diff --git a/api/src/server.js b/api/src/server.js index 65fa0b99..0013f23b 100644 --- a/api/src/server.js +++ b/api/src/server.js @@ -27,7 +27,14 @@ require('./widget-data'); const { dispatch } = require('./router'); -http.createServer(dispatch).listen(PORT, () => { +/* Loopback only. nginx is the only thing that ever calls this port, and it does + so on 127.0.0.1, so binding every interface only added a way into the API that + goes around nginx: reachable from anything sharing the container's network, + and arriving without the client address nginx resolves, so rate limiting saw + one caller. */ +const HOST = '127.0.0.1'; + +http.createServer(dispatch).listen(PORT, HOST, () => { const { CONFIG_PATH, ICONS_PATH } = require('./config'); const { getRegistry } = require('./widgets'); const pkg = require('../package.json'); diff --git a/api/test/environment-doc.test.js b/api/test/environment-doc.test.js index b9224627..9c59de9a 100644 --- a/api/test/environment-doc.test.js +++ b/api/test/environment-doc.test.js @@ -107,7 +107,9 @@ test('the documented defaults are the ones in the code', () => { test('the documented session and hash defaults match the code', () => { const defaults = Object.fromEntries(documentedRows()); const auth = read('api/src/auth.js'); - assert.equal(defaults.SESSION_MAX_AGE_DAYS, /_maxAgeDays > 0 \? _maxAgeDays : (\d+)/.exec(auth)[1]); + /* Documented in hours, because the default is shorter than the day the + variable is named for. */ + assert.equal(defaults.SESSION_MAX_AGE_DAYS, `${/const DEFAULT_MAX_AGE_HOURS = (\d+)/.exec(auth)[1]}h`); assert.equal(defaults.PASSWORD_HASH_MEMORY, /const DEFAULT_PROFILE = '([\w]+)'/.exec(auth)[1]); }); diff --git a/api/test/http-integration.test.js b/api/test/http-integration.test.js index b3a77100..f837ff4b 100644 --- a/api/test/http-integration.test.js +++ b/api/test/http-integration.test.js @@ -35,7 +35,7 @@ require('../src/routes'); // registers auth/config/health/badges/... + OPTIONS require('../src/widget-data'); // registers /api/widget-data/:id (pulls in widgets) const { dispatch, on } = require('../src/router'); const { saveConfig, loadConfig } = require('../src/config'); -const { hashPassword, makeToken, clearAttempts } = require('../src/auth'); +const { hashPassword, makeToken, verifyToken, clearAttempts, RENEW_AFTER_MS } = require('../src/auth'); /* Routes that fail on purpose, to prove the router turns a handler error into a 500 instead of crashing the process. */ @@ -66,8 +66,11 @@ after( }), ); +/* Origin defaults to this server's own, which is what a browser sends on a + same-origin write and what the origin check now requires. Pass `origin: null` + for the case of a request that states none. */ function req(method, pathname, opts = {}) { - const { cookie, body, origin, host } = opts; + const { cookie, body, origin = base, host } = opts; const data = body != null ? JSON.stringify(body) : null; const u = new URL(base + pathname); return new Promise((resolve, reject) => { @@ -237,6 +240,53 @@ test('cross-origin login, logout, ping and badge-proxy are rejected', async () = assert.equal(badge.status, 403); }); +/* A browser states an origin on every write, including a same-origin one, so a + write that states none did not come from a page on this dashboard. */ +test('a write that states no origin at all is rejected', async () => { + for (const [pathname, body] of [ + ['/api/config', { items: [] }], + ['/api/ping', { url: 'http://x' }], + ['/api/badge-proxy', { url: 'http://x' }], + ['/api/auth/logout', {}], + ['/api/auth/login', { password: 'correct-horse' }], + ]) { + const r = await req('POST', pathname, { cookie: validCookie, body, origin: null }); + assert.equal(r.status, 403, pathname); + assert.match(String(r.body?.error), /Origin/, pathname); + } +}); + +test('a read is not asked for an origin, since it changes nothing', async () => { + const r = await req('GET', '/api/config', { cookie: validCookie, origin: null }); + assert.equal(r.status, 200); +}); + +/* Signed by the server, so it cannot be backdated by the holder: this builds the + token the server would have issued at that moment. */ +function agedCookie(ms) { + const real = Date.now; + Date.now = () => real() - ms; + try { + return 'ds=' + makeToken('session-aged', SECRET); + } finally { + Date.now = real; + } +} + +test('a session still in use is extended rather than expiring under the shorter window', async () => { + const r = await req('GET', '/api/config', { cookie: agedCookie(RENEW_AFTER_MS + 1000) }); + assert.equal(r.status, 200); + const issued = String(r.headers['set-cookie'] || ''); + assert.match(issued, /ds=.+/, 'a session past halfway must come back renewed'); + assert.equal(verifyToken(/ds=([^;]+)/.exec(issued)[1], SECRET), 'session-aged'); +}); + +test('a fresh session is not handed a new cookie on every request', async () => { + const r = await req('GET', '/api/config', { cookie: validCookie }); + assert.equal(r.status, 200); + assert.equal(r.headers['set-cookie'], undefined); +}); + /* Config import/export route. */ test('POST /api/config rejects items that are not an array', async () => { const r = await req('POST', '/api/config', { cookie: validCookie, body: { items: 'nope' } }); @@ -379,10 +429,21 @@ test('GET /api/truenas-proxy is not routed', async () => { test('dismiss-setup records the flag for an authenticated same-origin request', async () => { const r = await req('POST', '/api/auth/dismiss-setup', { cookie: validCookie }); assert.equal(r.status, 200); - const check = await req('GET', '/api/auth/check'); + const check = await req('GET', '/api/auth/check', { cookie: validCookie }); assert.equal(check.body.setupPrompted, true); }); +/* The one route that answers before sign-in, so what it volunteers is the whole + pre-auth surface. */ +test('the pre-auth check withholds how the install is configured', async () => { + const r = await req('GET', '/api/auth/check'); + assert.equal(r.status, 200); + assert.equal(r.body.enabled, true); + assert.equal(r.body.authenticated, false); + assert.ok(!('passwordSet' in r.body), 'passwordSet must not be told to a caller with no session'); + assert.ok(!('setupPrompted' in r.body), 'setupPrompted must not be told to a caller with no session'); +}); + test('set-password rejects a too-short password', async () => { const r = await req('POST', '/api/auth/set-password', { cookie: validCookie, body: { password: 'short' } }); assert.equal(r.status, 400); diff --git a/api/test/security-doc.test.js b/api/test/security-doc.test.js index 090f88dc..1517b7f2 100644 --- a/api/test/security-doc.test.js +++ b/api/test/security-doc.test.js @@ -68,12 +68,19 @@ test('the documented login limit is the one in the code', () => { }); test('the documented session lifetime is the one in the code', () => { - const m = /_maxAgeDays > 0 \? _maxAgeDays : (\d+)/.exec(auth); + const m = /const DEFAULT_MAX_AGE_HOURS = (\d+)/.exec(auth); assert.ok(m, 'the default session lifetime was not found in auth.js'); - assert.match(doc, new RegExp(`default ${m[1]} days`)); + assert.match(doc, new RegExp(`default ${m[1]} hours`)); assert.match(doc, /SESSION_MAX_AGE_DAYS/); }); +/* The lifetime only bounds an unused token if a used one is reissued, so the + page may only claim the renewal while the code still does it. */ +test('the documented session renewal is the one in the code', () => { + assert.match(auth, /RENEW_AFTER_MS = SESSION_MAX_AGE_MS \/ 2/, 'auth.js no longer renews at the halfway mark'); + assert.match(doc, /halfway/); +}); + /* ── the client address ───────────────────────────────────────────────────── */ /* The reason matters as much as the fact. nginx overwriting the header is true diff --git a/api/test/session-renewal.test.js b/api/test/session-renewal.test.js new file mode 100644 index 00000000..ab501630 --- /dev/null +++ b/api/test/session-renewal.test.js @@ -0,0 +1,128 @@ +/* The shortened session is an idle window, not a fixed one. + + Shortening the lifetime on its own would sign people out mid-task, so a + session still in use is reissued once it is past halfway. What the lifetime + then bounds is how long a token nobody is using stays valid, which is the + thing that matters when one leaks. */ +const { tmpPath } = require('../test-support/tmp'); +process.env.CONFIG_PATH = tmpPath('apps.json'); + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const { EventEmitter } = require('node:events'); +const { + makeToken, + readToken, + verifyToken, + refreshSession, + newSessionId, + newSessionSecret, + SESSION_MAX_AGE_MS, + RENEW_AFTER_MS, +} = require('../src/auth'); +const { saveConfig } = require('../src/config'); + +const HOURS = 60 * 60 * 1000; + +test('the default idle window is hours, not the month it used to be', () => { + assert.equal(SESSION_MAX_AGE_MS, 12 * HOURS); + assert.equal(RENEW_AFTER_MS, SESSION_MAX_AGE_MS / 2); +}); + +test('readToken reports when a session was issued', () => { + const secret = newSessionSecret(); + const id = newSessionId(); + const before = Date.now(); + const read = readToken(makeToken(id, secret), secret); + assert.equal(read.sessionId, id); + assert.ok(read.iat >= before && read.iat <= Date.now()); +}); + +test('readToken refuses a token signed with another secret', () => { + const token = makeToken(newSessionId(), newSessionSecret()); + assert.equal(readToken(token, newSessionSecret()), null); + assert.equal(verifyToken(token, newSessionSecret()), null); +}); + +/* Signed, so it cannot be backdated by the holder: this builds one the way the + server would have at that moment. */ +function tokenAgedBy(ms, secret, id) { + const real = Date.now; + Date.now = () => real() - ms; + try { + return makeToken(id, secret); + } finally { + Date.now = real; + } +} + +test('a token past its window no longer verifies', () => { + const secret = newSessionSecret(); + const old = tokenAgedBy(SESSION_MAX_AGE_MS + 1000, secret, newSessionId()); + assert.equal(readToken(old, secret), null); +}); + +function reqRes(cookie) { + const req = { headers: cookie ? { cookie: `ds=${cookie}` } : {}, socket: {} }; + const res = Object.assign(new EventEmitter(), { + headers: {}, + setHeader(k, v) { + this.headers[k.toLowerCase()] = v; + }, + }); + return { req, res }; +} + +function authOn() { + const secret = newSessionSecret(); + saveConfig({ items: [], settings: { auth: { enabled: true, passwordHash: 'x', secret } } }); + return secret; +} + +test('a fresh session is left alone', () => { + const secret = authOn(); + const { req, res } = reqRes(makeToken(newSessionId(), secret)); + assert.equal(refreshSession(req, res), false); + assert.equal(res.headers['set-cookie'], undefined); +}); + +test('a session past halfway is reissued under the same identifier', () => { + const secret = authOn(); + const id = newSessionId(); + const { req, res } = reqRes(tokenAgedBy(RENEW_AFTER_MS + 1000, secret, id)); + assert.equal(refreshSession(req, res), true); + const issued = /ds=([^;]+)/.exec(res.headers['set-cookie'])[1]; + const read = readToken(issued, secret); + assert.equal(read.sessionId, id, 'renewal must extend the session, not start a new one'); + assert.ok(Date.now() - read.iat < 1000, 'the reissued token must carry a fresh issued-at'); +}); + +test('the reissued cookie keeps the flags the original was set with', () => { + const secret = authOn(); + const { req, res } = reqRes(tokenAgedBy(RENEW_AFTER_MS + 1000, secret, newSessionId())); + refreshSession(req, res); + const cookie = res.headers['set-cookie']; + assert.match(cookie, /HttpOnly/); + assert.match(cookie, /SameSite=Strict/); + assert.match(cookie, /Path=\//); +}); + +test('an expired session is not renewed back to life', () => { + const secret = authOn(); + const { req, res } = reqRes(tokenAgedBy(SESSION_MAX_AGE_MS + 1000, secret, newSessionId())); + assert.equal(refreshSession(req, res), false); + assert.equal(res.headers['set-cookie'], undefined); +}); + +test('nothing is renewed when there is no session to renew', () => { + authOn(); + const { req, res } = reqRes(null); + assert.equal(refreshSession(req, res), false); +}); + +test('nothing is renewed while authentication is off', () => { + const secret = newSessionSecret(); + saveConfig({ items: [], settings: { auth: { enabled: false, secret } } }); + const { req, res } = reqRes(tokenAgedBy(RENEW_AFTER_MS + 1000, secret, newSessionId())); + assert.equal(refreshSession(req, res), false); +}); diff --git a/api/test/skip-tls-scope.test.js b/api/test/skip-tls-scope.test.js new file mode 100644 index 00000000..14d87375 --- /dev/null +++ b/api/test/skip-tls-scope.test.js @@ -0,0 +1,105 @@ +/* Turning certificate checking off is scoped to your own network, wherever the + request came from. + + The server-wide setting was already scoped this way. The per-app switch and the + per-request flag were not: both were handed straight to the transport, so a + badge pointed at a public HTTPS service could be polled with verification off, + and that is the request that carries a stored credential. + + The config file is written before proxy.js is required, so the global-setting + branch reads a real file rather than a stub. */ +const fs = require('node:fs'); +const { tmpPath } = require('../test-support/tmp'); + +const CONFIG_PATH = tmpPath('apps.json'); +process.env.CONFIG_PATH = CONFIG_PATH; +fs.writeFileSync(CONFIG_PATH, JSON.stringify({ items: [], settings: { server: { skipTlsVerify: true } } })); + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const https = require('node:https'); +const { EventEmitter } = require('node:events'); +const { resolveSkipTls, SKIP_TLS_IGNORED_MESSAGE, _internals } = require('../src/proxy'); + +const INTERNAL = ['127.0.0.1', '192.168.1.10', '10.0.0.5', 'localhost', 'socket-proxy', '[::1]']; +const PUBLIC = ['api.github.com', 'example.com', '8.8.8.8', '1.1.1.1']; + +test('an explicit request to skip is honoured only for an internal host', () => { + for (const h of INTERNAL) { + assert.deepEqual(resolveSkipTls(h, true), { skip: true, ignored: false }, h); + } + for (const h of PUBLIC) { + assert.deepEqual(resolveSkipTls(h, true), { skip: false, ignored: true }, h); + } +}); + +test('not asking to skip is never overridden into skipping', () => { + for (const h of [...INTERNAL, ...PUBLIC]) { + assert.deepEqual(resolveSkipTls(h, false), { skip: false, ignored: false }, h); + } +}); + +/* No per-request answer, so the stored setting decides. It is on in this file's + config, and is scoped the same way. */ +test('the server-wide setting is scoped to internal hosts too', () => { + assert.equal(resolveSkipTls('192.168.1.10', undefined).skip, true); + assert.equal(resolveSkipTls('api.github.com', undefined).skip, false); + assert.equal(resolveSkipTls('api.github.com', undefined).ignored, true); +}); + +/* The decision is only worth anything if it reaches the transport, so these read + the options actually handed to https.request. */ +function captureRequest(t) { + const calls = []; + const original = https.request; + https.request = (opts, _cb) => { + calls.push(opts); + const req = new EventEmitter(); + req.end = () => {}; + req.write = () => {}; + req.destroy = () => {}; + setImmediate(() => req.emit('error', Object.assign(new Error('stub'), { code: 'DEPTH_ZERO_SELF_SIGNED_CERT' }))); + return req; + }; + t.after(() => { + https.request = original; + }); + return calls; +} + +test('fetchJSON keeps certificate checking on for a public host that asked to skip', async t => { + const calls = captureRequest(t); + await _internals.fetchJSON('https://api.github.com/x', { skipTls: true }).catch(() => {}); + assert.equal(calls[0].rejectUnauthorized, true); +}); + +test('fetchJSON turns certificate checking off for an internal host that asked to skip', async t => { + const calls = captureRequest(t); + await _internals.fetchJSON('https://192.168.1.10/x', { skipTls: true }).catch(() => {}); + assert.equal(calls[0].rejectUnauthorized, false); +}); + +test('a certificate failure on a public host explains that the skip did not apply', async t => { + captureRequest(t); + const err = await _internals.fetchJSON('https://api.github.com/x', { skipTls: true }).catch(e => e); + /* Vouched rather than left on the message, because api-error.js replaces an + unvouched message with a generic one. */ + assert.equal(err.vouchedMessage, SKIP_TLS_IGNORED_MESSAGE); +}); + +test('a certificate failure nobody asked to skip is not explained away', async t => { + captureRequest(t); + const err = await _internals.fetchJSON('https://api.github.com/x', { skipTls: false }).catch(e => e); + assert.equal(err.vouchedMessage, undefined); +}); + +test('pingUrl scopes the flag the same way and says so on a certificate failure', async t => { + const calls = captureRequest(t); + const pub = await _internals.pingUrl('https://api.github.com/', 500, true); + assert.equal(calls[0].rejectUnauthorized, true); + assert.equal(pub.error, SKIP_TLS_IGNORED_MESSAGE); + + const internal = await _internals.pingUrl('https://192.168.1.10/', 500, true); + assert.equal(calls[1].rejectUnauthorized, false); + assert.notEqual(internal.error, SKIP_TLS_IGNORED_MESSAGE); +}); diff --git a/docker-compose.yml b/docker-compose.yml index 6fe48ac1..c32884ea 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -61,8 +61,9 @@ services: # - WIDGETS_PATH=/usr/share/nginx/html/widgets # Run as a read-only public showcase. See docs/demo.md. # - DEMO_MODE=true - # Session lifetime in days before re-login is required (default 30) - # - SESSION_MAX_AGE_DAYS=30 + # Idle session lifetime in days before re-login is required (default 12 + # hours). Accepts a fraction. A session in use is extended automatically. + # - SESSION_MAX_AGE_DAYS=0.5 healthcheck: test: - CMD diff --git a/docs/architecture.md b/docs/architecture.md index 38cb8489..3268fe2b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -42,7 +42,7 @@ Everything is optional; the defaults are what the container ships with. | `TRUSTED_PROXY` | unset | Where a front proxy sits, so nginx can resolve the real client for rate limiting. See [security.md](./security.md#authentication). | | `ALLOW_PRIVATE_IPS` | unset | Turn the SSRF guard off, which most homelab installs need. See [security.md](./security.md#ssrf-guard). | | `PASSWORD_HASH_MEMORY` | `16mib` | Memory per password hash. See [security.md](./security.md#password-hashing). | -| `SESSION_MAX_AGE_DAYS` | `30` | Session lifetime before re-login. | +| `SESSION_MAX_AGE_DAYS` | `12h` | Idle session lifetime before re-login; accepts a fraction. A session in use is extended. | | `SOCKET_PROXY_URL` | unset | A Docker socket proxy, for container health monitoring. | | `DEMO_MODE` | unset | Run as a read-only public showcase. See [demo.md](./demo.md). | diff --git a/docs/security.md b/docs/security.md index 359241a0..9b4ddca7 100644 --- a/docs/security.md +++ b/docs/security.md @@ -63,10 +63,12 @@ Setting `ALLOW_PRIVATE_IPS=true` disables this guard entirely, so private, loopb - Passwords are hashed with scrypt and a per-password salt. - Session tokens are HMAC-signed and verified with a constant-time comparison. -- Session tokens carry a signed issued-at and expire after a fixed lifetime - (default 30 days). Override with `SESSION_MAX_AGE_DAYS`. Upgrading to this - version invalidates any session issued before it, so existing users log in - once more. +- Session tokens carry a signed issued-at and expire after an idle lifetime + (default 12 hours). Override with `SESSION_MAX_AGE_DAYS`, which accepts a + fraction. A session that is still being used is reissued once it passes the + halfway mark, so the lifetime bounds how long an unused token stays valid + rather than how long you may stay signed in. Upgrading to this version + invalidates any session issued before it, so existing users log in once more. - Login is rate-limited to 5 attempts per IP per 15 minutes. Rate-limit counters are held in memory, so a restart clears them and they are not shared across replicas. Run a single instance behind any proxy. diff --git a/ui/admin/index.html b/ui/admin/index.html index 35386184..a3e3c58e 100644 --- a/ui/admin/index.html +++ b/ui/admin/index.html @@ -8,7 +8,7 @@ Stackyard Settings - + @@ -285,6 +285,6 @@

General

- + diff --git a/ui/css/admin.css b/ui/css/admin.css index 2dadc2b9..6259afcd 100644 --- a/ui/css/admin.css +++ b/ui/css/admin.css @@ -355,6 +355,8 @@ html{background:var(--bg-color,var(--bg-outer))} .dlg-ul{list-style:none;margin:0;padding:0 12px 8px;display:flex;flex-direction:column;gap:4px} .dlg-li{display:flex;align-items:baseline;gap:8px;font-size:13px} .dlg-li .dlg-why{color:var(--dm);font-size:12px} +.dlg-choice{display:flex;align-items:flex-start;gap:8px;padding:0 12px 4px;font-size:13px;cursor:pointer} +.dlg-choice input{margin-block-start:2px;flex:none} .dlg-empty{padding:24px;text-align:center;color:var(--dm);font-size:13px} .fp-row{display:flex;align-items:center;gap:10px;padding:10px 12px;border-radius:var(--rs);background:none;border:none;color:var(--tx);font-size:14px;cursor:pointer;width:100%;text-align:start;font-family:inherit;transition:background var(--t)} .fp-row:hover{background:rgba(255,255,255,.06)} diff --git a/ui/js/admin.js b/ui/js/admin.js index 0d075a29..9ae578f3 100644 --- a/ui/js/admin.js +++ b/ui/js/admin.js @@ -15,7 +15,7 @@ import { canJoinFolder, applyDrop } from '/js/admin-drag-logic.js?v=53aeaa55'; under a different name. */ import { openModal as openDialog, confirmModal, promptModal } from '/js/modal.js?v=684b74e0'; import { parseYaml, YamlLiteError } from '/js/yaml-lite.js?v=14aa75ec'; -import { detectSource, convert, SKIP, NOTE } from '/js/import-foreign.js?v=cf38ec0b'; +import { detectSource, convert, insecureApps, clearSkipTls, SKIP, NOTE } from '/js/import-foreign.js?v=104555d0'; /* Admin UI: Stackyard Dashboard */ @@ -1380,6 +1380,38 @@ el('imp-foreign').onchange = async e => { skipped.map(s => ({ name: s.name, group: s.group, why: t(SKIP_TEXT[s.reason], { detail: s.detail || '' }) })), ); + /* A file from another dashboard can ask for certificate checking to be + skipped. That is a security setting, so it is never taken from the file on + the file's say-so: the apps that asked are named, and it stays off unless + it is turned on here. */ + const insecure = insecureApps(items); + /** @type {HTMLInputElement|null} */ + let skipTlsChoice = null; + if (insecure.length) { + const heading = document.createElement('div'); + heading.className = 'dlg-sec'; + heading.textContent = `${t('app.allowSelfSigned')} (${insecure.length})`; + const choice = document.createElement('label'); + choice.className = 'dlg-choice'; + skipTlsChoice = document.createElement('input'); + skipTlsChoice.type = 'checkbox'; + const why = document.createElement('span'); + why.className = 'dlg-why'; + why.textContent = t('app.selfSignedTip'); + choice.append(skipTlsChoice, why); + const ul = document.createElement('ul'); + ul.className = 'dlg-ul'; + for (const item of insecure) { + const li = document.createElement('li'); + li.className = 'dlg-li'; + const nm = document.createElement('span'); + setUserText(nm, item.label); + li.appendChild(nm); + ul.appendChild(li); + } + body.append(heading, choice, ul); + } + const ok = await confirmModal({ title: t('importForeign.title'), body, @@ -1391,6 +1423,7 @@ el('imp-foreign').onchange = async e => { input.value = ''; return; } + if (skipTlsChoice && !skipTlsChoice.checked) clearSkipTls(items); /* Appended, never merged: nothing already on the dashboard is renamed, reordered or removed by an import from somewhere else. */ await appendAndSave(items); diff --git a/ui/js/import-foreign.js b/ui/js/import-foreign.js index 2584c0b1..447672bf 100644 --- a/ui/js/import-foreign.js +++ b/ui/js/import-foreign.js @@ -428,6 +428,24 @@ function result(col) { return { items: col.items, skipped: col.skipped, notes: col.notes }; } +/* Skipping certificate checking is the one security setting a foreign file can + ask for, so it is the one the import has to ask about rather than carry across + on the file's say-so. The conversion still records what the file asked for; + these two are how the dialog names those apps and how it applies the answer. + + @param {any[]} items @returns {any[]} */ +export function insecureApps(items) { + return (Array.isArray(items) ? items : []).filter(i => i && i.type === 'app' && i.skipTlsVerify); +} + +/** Turn the request down, leaving each app exactly as one that never asked: the + conversion drops the field rather than storing a false. + @param {any[]} items @returns {any[]} */ +export function clearSkipTls(items) { + for (const app of insecureApps(items)) delete app.skipTlsVerify; + return items; +} + /** Convert a parsed document of a detected kind. @param {'homepage-services'|'homepage-bookmarks'|'dashy'} kind @param {any} doc @param {Iterable} takenIds diff --git a/ui/test/import-foreign.test.mjs b/ui/test/import-foreign.test.mjs index 6c6dbc15..c86903b4 100644 --- a/ui/test/import-foreign.test.mjs +++ b/ui/test/import-foreign.test.mjs @@ -18,6 +18,8 @@ const { convert, convertIcon, convertDashy, + insecureApps, + clearSkipTls, convertHomepageServices, convertHomepageBookmarks, SKIP, @@ -401,3 +403,50 @@ test('the caller sees the ids a conversion allocated', () => { const out = convertHomepageServices(parseYaml(SERVICES), taken); for (const item of out.items) assert.ok(taken.has(item.id), 'the id is in the caller’s set'); }); + +/* A file from another dashboard can ask for certificate checking to be skipped. + The import names those apps and asks, rather than taking a security setting on + the file's say-so, so the conversion has to keep the request separable from + the decision. */ +test('the apps a file asked to skip certificate checking for are identifiable', () => { + const out = convertDashy( + parseYaml(` +sections: + - name: Net + items: + - title: PVE + url: https://pve.lan:8006 + statusCheck: true + statusCheckAllowInsecure: true + - title: Plain + url: https://plain.lan + statusCheck: true +`), + ); + assert.deepEqual( + insecureApps(out.items).map(a => a.label), + ['PVE'], + ); +}); + +test('turning the request down leaves the app as one that never asked', () => { + const items = [ + { id: 'a', type: 'app', label: 'PVE', skipTlsVerify: true }, + { id: 'b', type: 'app', label: 'Plain' }, + { id: 'c', type: 'folder', label: 'Net' }, + ]; + clearSkipTls(items); + assert.deepEqual(items[0], { id: 'a', type: 'app', label: 'PVE' }); + assert.deepEqual(insecureApps(items), []); +}); + +test('accepting the request is what keeps it, and nothing else is touched', () => { + const items = [{ id: 'a', type: 'app', label: 'PVE', skipTlsVerify: true }]; + assert.equal(insecureApps(items).length, 1); + assert.equal(items[0].skipTlsVerify, true); +}); + +test('insecureApps tolerates the shapes a damaged conversion could produce', () => { + assert.deepEqual(insecureApps(null), []); + assert.deepEqual(insecureApps([null, undefined, { type: 'app' }, { type: 'folder', skipTlsVerify: true }]), []); +}); From 0dae214df5d64737d99f81170f9cf8f387c61c5c Mon Sep 17 00:00:00 2001 From: SandObserver <260779319+SandObserver@users.noreply.github.com> Date: Tue, 11 Aug 2026 21:21:13 -0400 Subject: [PATCH 2/3] Seed the end-to-end config the way a browser writes it The seeding helper posts through an API request context, which is not a page and so states no origin. Writes now require one, which refused every seed. The address comes from the same definition the Playwright config resolves relative paths against, so the two cannot drift apart. --- e2e/base-url.js | 11 +++++++++++ e2e/helpers.js | 7 ++++++- playwright.config.js | 3 +-- 3 files changed, 18 insertions(+), 3 deletions(-) create mode 100644 e2e/base-url.js diff --git a/e2e/base-url.js b/e2e/base-url.js new file mode 100644 index 00000000..4f9f19b0 --- /dev/null +++ b/e2e/base-url.js @@ -0,0 +1,11 @@ +// @ts-check +/* The address the specs run against, defined once. + + Both the Playwright config and the seeding helper need it: the config to + resolve relative paths, the helper to state an origin on a write. Defining it + in each would let one point somewhere the other does not, and the failure + that produces is a rejected write rather than an obvious misconfiguration. */ + +const BASE_URL = process.env.BASE_URL || 'http://127.0.0.1:8730'; + +module.exports = { BASE_URL }; diff --git a/e2e/helpers.js b/e2e/helpers.js index 88acc9d2..95b47ce4 100644 --- a/e2e/helpers.js +++ b/e2e/helpers.js @@ -10,11 +10,16 @@ the half that actually breaks. */ const { expect } = require('@playwright/test'); +const { BASE_URL } = require('./base-url'); /** Replace the whole config. @param {import('@playwright/test').APIRequestContext} request */ async function seedConfig(request, config) { const body = { _schemaVersion: 3, items: [], settings: {}, ...config }; - const res = await request.post('/api/config', { data: body }); + /* A write has to state its origin, and this request context is not a page, so + nothing sets one for it. Seeding the way a browser writes is also the point: + a seed that could reach the API by a route the admin page cannot would be + testing something the product does not do. */ + const res = await request.post('/api/config', { data: body, headers: { Origin: BASE_URL } }); expect(res.ok(), `seeding failed: ${res.status()} ${await res.text()}`).toBeTruthy(); } diff --git a/playwright.config.js b/playwright.config.js index 0a5e7cd5..3c90ea92 100644 --- a/playwright.config.js +++ b/playwright.config.js @@ -15,8 +15,7 @@ ships, nginx and its headers included. */ const { defineConfig, devices } = require('@playwright/test'); - -const baseURL = process.env.BASE_URL || 'http://127.0.0.1:8730'; +const { BASE_URL: baseURL } = require('./e2e/base-url'); module.exports = defineConfig({ testDir: './e2e', From 8b65506d2065abcb54feab8536f3377c903dfc90 Mon Sep 17 00:00:00 2001 From: SandObserver <260779319+SandObserver@users.noreply.github.com> Date: Tue, 11 Aug 2026 21:48:25 -0400 Subject: [PATCH 3/3] Fix: stop the admin UI announcing saves that failed save() already reported whether the write reached the server, but no caller read the result. A failed write toasted "Save failed" and was immediately overwritten by "Updated", "Added" or "Imported" in the same shared toast element, and the item editor closed on top of it. The list edits had the same gap from the other side: delete, reorder, drag-drop, the Settings hide toggle and every folder-membership change mutate the in-memory list and save after, with no path back if the save failed. The admin list then showed a dashboard the server did not have until a reload. Add snapshotItems and saveWithRevert to the save logic, take a snapshot before each mutation, and put the list back when the write does not land. The item editor now stays open on failure so the filled-in form is not discarded. The label-visibility toggles were wired straight to change with no catch, so a failure was an unhandled rejection with no message and the switch left in its new position. --- CHANGELOG.md | 11 +++++ ui/js/admin-save-logic.js | 33 ++++++++++++++ ui/js/admin-settings.js | 23 +++++++--- ui/js/admin.js | 71 ++++++++++++++++++++++++------- ui/test/admin-save-logic.test.mjs | 68 +++++++++++++++++++++++++++++ 5 files changed, 184 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fa76afab..f8dfa28b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Saving an app or widget no longer reports "Updated" when the write failed; the + editor stays open with the form intact. + +- A failed save no longer leaves the admin list showing a delete, reorder, + drag-drop or folder change the server never received. + +- Importing a config reports "Imported" only once the write has landed. + +- A failed label-visibility toggle now reports the error instead of failing + silently with the switch left in its new position. + - The Books, Now Playing and Backup widgets now show why they failed, such as a missing API key or a rejected password, instead of a generic error. diff --git a/ui/js/admin-save-logic.js b/ui/js/admin-save-logic.js index f0e1dd49..b524f0cb 100644 --- a/ui/js/admin-save-logic.js +++ b/ui/js/admin-save-logic.js @@ -45,6 +45,39 @@ export function newItemId(label, fallback = 'item', taken = []) { return `${stem}_${n}`; } +/** A detached copy of the item list, taken before a change so it can be put + back. Config items are JSON by definition, so a round trip clones them + faithfully. + + @param {any[]} items @returns {any[]} */ +export function snapshotItems(items) { + return Array.isArray(items) ? JSON.parse(JSON.stringify(items)) : []; +} + +/** Run `write` and undo the local change when it did not reach the server. + + Every list edit in the admin UI changes the in-memory list first and saves + after. Without this a failed write leaves the list showing a dashboard the + server does not have, and the only sign of it is a toast that clears in + three seconds. + + `write` reports failure by resolving false; a throw is also a failure, and + is re-raised after the restore so the caller still sees it. + + @template T + @param {{ write: () => Promise, snapshot: T, + restore: (snapshot: T) => void }} opts + @returns {Promise} */ +export async function saveWithRevert({ write, snapshot, restore }) { + let ok = false; + try { + ok = (await write()) !== false; + } finally { + if (!ok) restore(snapshot); + } + return ok; +} + /* Returns { error } or { item }. */ /** Put `item` where the item with `id` currently is, or append it. diff --git a/ui/js/admin-settings.js b/ui/js/admin-settings.js index 623b1d4d..68511878 100644 --- a/ui/js/admin-settings.js +++ b/ui/js/admin-settings.js @@ -230,12 +230,23 @@ export function showBgFields(type) { const brRow = el('bg-brightness-row'); if (brRow) brRow.classList.toggle('d-none', type === 'color'); } -async function saveLabels() { - const c = await ag('/api/config'); - c.settings = c.settings || {}; - c.settings.showLabels = { desktop: inp('set-lbl-d')?.checked !== false, ios: inp('set-lbl-m')?.checked || false }; - await ap('/api/config', c); - toast(t('toast.saved')); +/** @param {Event} [e] */ +async function saveLabels(e) { + const toggled = /** @type {HTMLInputElement|null} */ (e?.target ?? null); + const wasChecked = toggled ? toggled.checked : false; + try { + const c = await ag('/api/config'); + c.settings = c.settings || {}; + c.settings.showLabels = { desktop: inp('set-lbl-d')?.checked !== false, ios: inp('set-lbl-m')?.checked || false }; + await ap('/api/config', c); + toast(t('toast.saved')); + } catch (err) { + /* Wired straight to `change`, so an unhandled failure left the box in its + new position showing a setting the server was never given. Assigning + `checked` fires no event, so this does not loop. */ + if (toggled) toggled.checked = !wasChecked; + toast(t('toast.saveFailed', { err: err.message }), 'err'); + } } async function saveWallpaper() { try { diff --git a/ui/js/admin.js b/ui/js/admin.js index 9ae578f3..08f969ec 100644 --- a/ui/js/admin.js +++ b/ui/js/admin.js @@ -2,7 +2,14 @@ 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=84d58686'; import { html, raw, setHtml } from '/js/html.js?v=ccec347c'; import { reorderItems, resolveAdminSection } from '/js/admin-logic.js?v=3c60f7b0'; -import { newItemId, buildAppItem, upsertItem, claimFolderChildren } from '/js/admin-save-logic.js?v=77cac1d1'; +import { + newItemId, + buildAppItem, + upsertItem, + claimFolderChildren, + snapshotItems, + saveWithRevert, +} from '/js/admin-save-logic.js?v=77cac1d1'; import { API, toast, ag, ap, initInlineEdit } from '/js/admin-shared.js?v=bb36dbb3'; import { checkAuth, wirePasswordStrength } from '/js/admin-auth.js?v=dd849d4c'; import { state } from '/js/admin-state.js?v=3f9ad806'; @@ -135,8 +142,28 @@ async function appendAndSave(newItems) { } } +/** Save, and put the list back if the write did not land. Never rejects: every + caller here is an event handler, and `save` has already said what went + wrong. Returns whether the change is now on the server, so a caller does not + report its own success over a failed write. */ +async function saveOrRevert(before) { + try { + return await saveWithRevert({ + write: save, + snapshot: before, + restore: items => { + state.items = items; + render(); + }, + }); + } catch { + return false; + } +} + function moveRow(item, dir, opts = {}) { - if (reorderItems(state.items, item, dir, opts)) save(); + const before = snapshotItems(state.items); + if (reorderItems(state.items, item, dir, opts)) saveOrRevert(before); } /* Constant markup only; no user data reaches these. */ @@ -310,9 +337,9 @@ function mkRow(item, idx, { indent = false, childIdx = null, folderId = null } = hb.title = lbl; hb.setAttribute('aria-label', lbl); hb.onclick = () => { + const before = snapshotItems(state.items); item.hidden = !item.hidden; - save(); - render(); + saveOrRevert(before); }; ac.append(hb); } else { @@ -360,6 +387,7 @@ function mkRow(item, idx, { indent = false, childIdx = null, folderId = null } = if (!raw) return; const drop = parseDragData(raw); if (!drop) return; + const before = snapshotItems(state.items); if ( applyDrop(state.items, { ...drop, @@ -371,7 +399,7 @@ function mkRow(item, idx, { indent = false, childIdx = null, folderId = null } = dropAbove, }) ) - save(); + saveOrRevert(before); }); if (document.documentElement.classList.contains('is-mobile')) wireTouchDrag(row, handle, { indent, folderId }); @@ -465,6 +493,7 @@ function wireTouchDrag(row, handle, { indent, folderId }) { end(); if (!tr) return; const into = tr.dataset.isFolder && canJoinFolder(itemType(srcId)); + const before = snapshotItems(state.items); const drop = applyDrop(state.items, { srcId, srcFolderId: indent ? folderId : null, @@ -475,7 +504,7 @@ function wireTouchDrag(row, handle, { indent, folderId }) { childIdx: tr.dataset.childIdx != null ? Number(tr.dataset.childIdx) : null, dropAbove: into ? false : dropAbove, }); - if (drop) save(); + if (drop) saveOrRevert(before); }; const cancel = () => end(); handle.addEventListener('pointermove', move); @@ -679,19 +708,21 @@ function openModal(idx) { showEditView(); } -function _evDelete(item, idx) { +async function _evDelete(item, idx) { if (!item) return; if (item.type === 'folder') { if (!confirm(t('confirm.deleteFolder', { name: item.label }))) return; } else { if (!confirm(t('confirm.remove', { name: item.label || item.id }))) return; } + const before = snapshotItems(state.items); state.items.forEach(f => { if (f.type === 'folder') f.children = (f.children || []).filter(id => id !== item.id); }); state.items.splice(idx, 1); - save().catch(() => {}); - showListView(); + /* Staying put on a failed delete: the list behind this view still holds the + item, so returning to it would show it as deleted. */ + if (await saveOrRevert(before)) showListView(); } { const s = inp('al-search'); @@ -753,12 +784,13 @@ function openFolderPicker(appId, targetFolderId = null) { } available.forEach(app => { const b = rowBtn('', () => { + const before = snapshotItems(state.items); state.items.forEach(f => { if (f.type === 'folder') f.children = (f.children || []).filter(id => id !== app.id); }); if (!tf.children) tf.children = []; tf.children.push(app.id); - save(); + saveOrRevert(before); close(); }); const ri = document.createElement('span'); @@ -778,10 +810,11 @@ function openFolderPicker(appId, targetFolderId = null) { }); } else { const none = rowBtn(currentFolder ? 'muted' : 'cur', () => { + const before = snapshotItems(state.items); state.items.forEach(f => { if (f.type === 'folder') f.children = (f.children || []).filter(id => id !== appId); }); - save(); + saveOrRevert(before); close(); }); const ns = document.createElement('span'); @@ -792,12 +825,13 @@ function openFolderPicker(appId, targetFolderId = null) { folders.forEach(f => { const cur = currentFolder?.id === f.id; const b = rowBtn(cur ? 'cur' : '', () => { + const before = snapshotItems(state.items); state.items.forEach(ff => { if (ff.type === 'folder') ff.children = (ff.children || []).filter(id => id !== appId); }); if (!f.children) f.children = []; if (!f.children.includes(appId)) f.children.push(appId); - save(); + saveOrRevert(before); close(); }); const nm = document.createElement('span'); @@ -831,11 +865,12 @@ function openFolderPicker(appId, targetFolderId = null) { 'folder', state.items.map(i => i.id), ); + const before = snapshotItems(state.items); state.items.push({ id: fid, type: 'folder', label: name, children: [appId] }); state.items.forEach(f => { if (f.type === 'folder' && f.id !== fid) f.children = (f.children || []).filter(id => id !== appId); }); - save(); + saveOrRevert(before); }); const nrs = document.createElement('span'); nrs.textContent = '+ ' + t('folder.createNew'); @@ -958,8 +993,12 @@ async function doSave(orig) { } /* By id, not by position: an item that moved is still found, and one that has gone is appended rather than silently dropping the edit. */ + const before = snapshotItems(state.items); const { replaced } = upsertItem(state.items, state.eid, item); - await save(); + /* The editor stays open on a failed write, with the filled-in form intact: + closing it would discard the work and leave "Updated" as the last thing + said about it. */ + if (!(await saveOrRevert(before))) return; closeModal(); toast(replaced ? 'Updated' : 'Added'); } catch (e) { @@ -1242,9 +1281,9 @@ el('imp').onchange = async e => { tgt(e).value = ''; return; } + const before = snapshotItems(state.items); state.items = d.items; - await save(); - toast(t('toast.imported')); + if (await saveOrRevert(before)) toast(t('toast.imported')); } catch (e) { toast(t('toast.importFailed', { err: e.message }), 'err'); } diff --git a/ui/test/admin-save-logic.test.mjs b/ui/test/admin-save-logic.test.mjs index 57e31f6d..fe635be2 100644 --- a/ui/test/admin-save-logic.test.mjs +++ b/ui/test/admin-save-logic.test.mjs @@ -7,6 +7,8 @@ import { upsertItem, claimFolderChildren, randomSuffix, + snapshotItems, + saveWithRevert, } from '../js/admin-save-logic.js'; test('cleanId keeps alphanumerics, collapses the rest, and trims', () => { @@ -291,3 +293,69 @@ test('claimFolderChildren tolerates junk', () => { assert.doesNotThrow(() => claimFolderChildren(null, 'f', ['a'])); assert.doesNotThrow(() => claimFolderChildren([null, 'x', { id: 'f', type: 'folder' }], 'g', ['a'])); }); + +test('snapshotItems detaches nested folder children', () => { + const items = [{ id: 'f1', type: 'folder', children: ['a'] }]; + const copy = snapshotItems(items); + items[0].children.push('b'); + items[0].label = 'renamed'; + assert.deepEqual(copy, [{ id: 'f1', type: 'folder', children: ['a'] }]); +}); + +test('snapshotItems tolerates junk', () => { + assert.deepEqual(snapshotItems(null), []); + assert.deepEqual(snapshotItems(undefined), []); +}); + +test('saveWithRevert keeps the change when the write lands', async () => { + let restored = null; + const ok = await saveWithRevert({ + write: async () => true, + snapshot: ['before'], + restore: s => { + restored = s; + }, + }); + assert.equal(ok, true); + assert.equal(restored, null); +}); + +test('saveWithRevert puts the list back when the write reports failure', async () => { + let restored = null; + const ok = await saveWithRevert({ + write: async () => false, + snapshot: ['before'], + restore: s => { + restored = s; + }, + }); + assert.equal(ok, false); + assert.deepEqual(restored, ['before']); +}); + +test('saveWithRevert restores and re-raises when the write throws', async () => { + let restored = null; + await assert.rejects( + saveWithRevert({ + write: async () => { + throw new Error('offline'); + }, + snapshot: ['before'], + restore: s => { + restored = s; + }, + }), + /offline/, + ); + assert.deepEqual(restored, ['before']); +}); + +test('saveWithRevert treats a write that returns nothing as success', () => { + /* `save` is the only caller and returns a boolean, but a void write must not + be read as a failure and silently undone. */ + return saveWithRevert({ + write: async () => {}, + snapshot: ['before'], + restore: () => assert.fail('should not revert'), + }).then(ok => assert.equal(ok, true)); +});