diff --git a/CHANGELOG.md b/CHANGELOG.md index c3e00823..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. @@ -89,6 +100,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/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', 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-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 0d075a29..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'; @@ -15,7 +22,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 */ @@ -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'); } @@ -1380,6 +1419,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 +1462,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/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)); +}); 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 }]), []); +});