From 55eadee06c5c745be6e1510097867f66acdb94fe Mon Sep 17 00:00:00 2001 From: SandObserver <260779319+SandObserver@users.noreply.github.com> Date: Wed, 12 Aug 2026 14:39:22 -0400 Subject: [PATCH 1/7] trim comments: api/ --- api/src/api-error.js | 64 +++------ api/src/auth.js | 134 ++++++------------ api/src/badge-extract.js | 3 - api/src/badge-headers.js | 52 ++----- api/src/config-secrets.js | 33 +---- api/src/config.js | 44 ++---- api/src/demo-data.js | 4 - api/src/demo.js | 2 - api/src/icon-sniff.js | 5 - api/src/log.js | 27 +--- api/src/metrics.js | 23 +-- api/src/parse-multipart.js | 5 +- api/src/parse-prometheus.js | 10 +- api/src/parse-xml.js | 33 ++--- api/src/percent-decode.js | 10 +- api/src/poll-limits.js | 14 +- api/src/provider-dispatch.js | 10 -- api/src/proxy.js | 108 ++++++-------- api/src/router.js | 30 ++-- api/src/routes/auth.js | 65 +++------ api/src/routes/badges.js | 33 ++--- api/src/routes/config.js | 45 ++---- api/src/routes/health.js | 30 ++-- api/src/routes/icons.js | 40 ++---- api/src/routes/system.js | 28 ++-- api/src/routes/version.js | 20 +-- api/src/secret-scope.js | 21 +-- api/src/semver.js | 21 +-- api/src/server.js | 19 +-- api/src/svg-sanitize.js | 39 ++--- api/src/timeouts.js | 6 +- api/src/widget-data.js | 36 ++--- api/src/widget-secrets.js | 30 ++-- api/src/widget-settings.js | 14 +- api/src/widgets.js | 53 ++----- api/test-support/tmp.js | 23 --- api/test/api-error-integration.test.js | 9 -- api/test/api-error.test.js | 15 -- api/test/auth-hardening.test.js | 19 --- api/test/auth-lockout.test.js | 20 --- api/test/auth.test.js | 5 - api/test/backup-data.test.js | 2 - api/test/badge-headers.test.js | 5 - api/test/badges-route.test.js | 14 -- api/test/biome-schema.test.js | 15 -- api/test/blocked-ranges.test.js | 15 -- api/test/boot-output.test.js | 2 - api/test/client-ip.test.js | 21 --- api/test/config-durability.test.js | 18 --- api/test/config-kill.test.js | 2 - api/test/config.test.js | 5 - api/test/connections-url.test.js | 15 -- api/test/contributor-doc.test.js | 19 --- api/test/entrypoint-config-check.test.js | 2 - api/test/error-disclosure.test.js | 25 ---- api/test/health-route.test.js | 14 -- api/test/http-integration.test.js | 12 -- api/test/icon-upload-name.test.js | 18 --- api/test/image-hardening.test.js | 17 --- api/test/internal-host.test.js | 20 --- api/test/link-url.test.js | 26 ---- api/test/log.test.js | 6 - api/test/login-concurrency.test.js | 33 ----- api/test/metrics-parsing.test.js | 5 - api/test/module-system.test.js | 13 -- api/test/nginx-config.test.js | 26 ---- api/test/parse-prometheus.test.js | 6 - api/test/parse-xml.test.js | 2 - api/test/password-hash-format.test.js | 27 ---- api/test/percent-decode.test.js | 19 --- api/test/ping-log.test.js | 3 - api/test/poll-rate-limits.test.js | 20 --- api/test/proxy-boundary.test.js | 2 - api/test/proxy-ping-rewrite.test.js | 22 --- api/test/proxy-rewrite.test.js | 2 - api/test/release-workflow.test.js | 5 - api/test/request-deadline.test.js | 15 -- api/test/secret-scope-integration.test.js | 9 -- api/test/secret-scope.test.js | 13 -- api/test/security-doc.test.js | 21 --- api/test/semver.test.js | 12 -- api/test/session-revocation.test.js | 21 --- api/test/session-secret.test.js | 12 -- api/test/supervisord-fatal.test.js | 23 --- api/test/tmp-hygiene.test.js | 24 ---- api/test/unknown-widget-config.test.js | 19 --- .../unknown-widget-empty-registry.test.js | 8 -- api/test/url-scheme-policy.test.js | 25 ---- api/test/version-route.test.js | 18 --- api/test/widget-data.test.js | 3 - api/test/widget-demo.test.js | 2 - api/test/widget-error-route.test.js | 10 -- api/test/widget-error.test.js | 35 ----- api/test/widget-rejections.test.js | 17 --- api/test/widget-settings.test.js | 13 -- api/test/workflow-hardening.test.js | 21 --- 96 files changed, 318 insertions(+), 1663 deletions(-) diff --git a/api/src/api-error.js b/api/src/api-error.js index 88c73f03..c466a3e9 100644 --- a/api/src/api-error.js +++ b/api/src/api-error.js @@ -1,24 +1,22 @@ -/* Structured API errors: { error, kind, detail? }. `kind` is a closed set and a - frontend must treat an unknown or missing one as INTERNAL. `detail` carries - server-derived values only, never an upstream body, header or filesystem path. - Both contracts are specified in docs/api-errors.md. */ +/* Structured API errors: { error, kind, detail? }. `kind` is a closed set. + `detail` carries server-derived values only, never an upstream body, header or + filesystem path. See docs/api-errors.md. */ const { json } = require('./router'); const log = require('./log'); const KIND = Object.freeze({ - NETWORK: 'network' /* could not reach the target at all */, - TIMEOUT: 'timeout' /* reached or dialling, but ran out of time */, - BLOCKED: 'blocked' /* our own outbound guard refused the target */, - AUTH: 'auth' /* the caller's Stackyard session, not the upstream's */, - UPSTREAM: 'upstream' /* we reached it and it answered with an error status */, - INVALID: 'invalid' /* the request or its input was malformed */, - INTERNAL: 'internal' /* anything we have not classified */, + NETWORK: 'network', + TIMEOUT: 'timeout', + BLOCKED: 'blocked', + AUTH: 'auth', + UPSTREAM: 'upstream', + INVALID: 'invalid', + INTERNAL: 'internal', }); const KINDS = Object.freeze(Object.values(KIND)); -/* Socket- and DNS-level failures. Node puts these on `err.code`. */ const NETWORK_CODES = new Set([ 'ECONNREFUSED', 'ECONNRESET', @@ -33,8 +31,6 @@ const NETWORK_CODES = new Set([ 'EADDRNOTAVAIL', ]); -/* Reported as `network`: the connection did not come up. The precise code goes - in detail.code. */ const TLS_CODES = new Set([ 'CERT_HAS_EXPIRED', 'DEPTH_ZERO_SELF_SIGNED_CERT', @@ -47,10 +43,7 @@ const TLS_CODES = new Set([ const TIMEOUT_CODES = new Set(['ETIMEDOUT', 'ESOCKETTIMEDOUT', 'UND_ERR_HEADERS_TIMEOUT']); -/* For a route that knows more than classify() could infer. */ class ApiError extends Error { - /* @param {string} message - @param {{ kind?: string, status?: number, detail?: Record }} [opts] */ constructor(message, opts = {}) { const { kind = KIND.INTERNAL, status = 500, detail } = opts; super(message); @@ -61,16 +54,11 @@ class ApiError extends Error { } } -/* An error whose message reaches the browser verbatim, for a sentence the widget - author wrote such as "Set a Pi-hole password". Throwing it asserts the message - holds nothing but words the author chose: never interpolate an upstream - response, a URL, or an error from a fetch into one. - - Marked with a field rather than by instanceof, because a widget's data.js is - require()d across a boundary where constructor identity is not reliable. */ +/* The message reaches the browser verbatim. Never interpolate an upstream + response, a URL or a fetch error into one. Detected by the vouchedMessage + field, not by instanceof: a widget's data.js is required across a boundary + where constructor identity is not reliable. */ class WidgetError extends Error { - /* @param {string} message shown to the user verbatim - @param {{ kind?: string, detail?: Record }} [opts] */ constructor(message, opts = {}) { super(message); this.name = 'WidgetError'; @@ -80,19 +68,15 @@ class WidgetError extends Error { } } -/* True however the error was constructed. */ const hasVouchedMessage = e => !!e && typeof e === 'object' && typeof e.vouchedMessage === 'string' && e.vouchedMessage !== ''; -/* Order matters: an explicit `kind` on the error wins, so a deeper module can - override the inference. */ function classify(e) { if (e && typeof e.kind === 'string' && KINDS.includes(e.kind)) { return e.detail ? { kind: e.kind, detail: e.detail } : { kind: e.kind }; } - /* Matched by name: importing the class from proxy.js would point the - dependency the wrong way. */ + /* Matched by name. Importing proxy.js here makes a require cycle. */ if (e && e.name === 'SsrfBlockedError') return { kind: KIND.BLOCKED }; const code = e && typeof e.code === 'string' ? e.code : null; @@ -103,22 +87,15 @@ function classify(e) { if (code === 'ERR_INVALID_URL') return { kind: KIND.INVALID, detail: { code } }; } - /* The deadline is raised as a plain Error with no code. This message is ours, - not an upstream's. */ if (e instanceof Error && e.message === 'Timed out') return { kind: KIND.TIMEOUT }; - /* JSON.parse on a request body. */ if (e instanceof SyntaxError) return { kind: KIND.INVALID }; return { kind: KIND.INTERNAL }; } -/* Separate from sending, so a route with its own body shape can merge these in. */ -/** @param {unknown} e - @param {{ kind?: string, detail?: Record, error?: string }} [overrides] */ -/* Composed from the kind, never filtered from the original: an OS error message - names internal addresses and paths, and filtering it is fail-open. The precise - message is logged instead. */ +/* Compose the message from the kind. Never filter the original: an OS error + message names internal addresses and paths, and filtering it is fail-open. */ const SAFE_MESSAGES = Object.freeze({ [KIND.NETWORK]: 'Could not reach the service.', [KIND.TIMEOUT]: 'The service did not respond in time.', @@ -138,8 +115,7 @@ function errorBody(e, overrides = {}) { const { kind, detail } = classify(e); const finalKind = overrides.kind || kind; const body = { - /* Only a message the code vouched for. Anything else comes from the kind, - never from e.message. */ + /* Only a message the code vouched for. Never e.message. */ error: overrides.error != null ? overrides.error : hasVouchedMessage(e) ? e.vouchedMessage : safeMessage(finalKind), kind: finalKind, }; @@ -148,20 +124,16 @@ function errorBody(e, overrides = {}) { return body; } -/* `status` is the fallback; an error carrying its own keeps it. */ /** @param {import('http').ServerResponse} res @param {unknown} e @param {{ status?: number, kind?: string, detail?: Record, error?: string, extra?: Record }} [opts] */ function fail(res, e, opts = {}) { const { status = 502, kind, detail, error, extra } = opts; - /* Anything can be thrown, so the shape is checked rather than assumed. */ const thrown = /** @type {{ status?: unknown, message?: unknown }} */ (e && typeof e === 'object' ? e : {}); const code = (typeof thrown.status === 'number' && thrown.status) || status; const body = errorBody(e, { kind, detail, error }); - /* The response does not carry the original message, so this is the only record - of what failed. */ if (typeof thrown.message === 'string' && thrown.message && thrown.message !== body.error) { log.error('request failed', { kind: body.kind, status: code, error: thrown.message }); } diff --git a/api/src/auth.js b/api/src/auth.js index ebbe332f..4fbdd10a 100644 --- a/api/src/auth.js +++ b/api/src/auth.js @@ -3,23 +3,19 @@ const { loadConfig, saveConfig } = require('./config'); const { decodeOrRaw } = require('./percent-decode'); const log = require('./log'); -/* SECRET_BYTES is the signing key for the session HMAC. SESSION_ID_BYTES is the - identifier inside the token, which is not a credential on its own. */ const SECRET_BYTES = 32, SESSION_ID_BYTES = 24; const newSessionSecret = () => crypto.randomBytes(SECRET_BYTES).toString('hex'); const newSessionId = () => crypto.randomBytes(SESSION_ID_BYTES).toString('hex'); -/* Mutates and returns the block, so a caller can write to it directly. */ function _authBlock(cfg) { cfg.settings = cfg.settings || {}; cfg.settings.auth = cfg.settings.auth || {}; return cfg.settings.auth; } -/* Invalidates every outstanding session, including the caller's own, so the - caller must issue a fresh cookie in the same response. Returns the new - secret so it can. */ +/* Invalidates every outstanding session, including the caller's own. The caller + must issue a fresh cookie in the same response. */ function rotateSessionSecret() { const cfg = loadConfig(); const auth = _authBlock(cfg); @@ -28,8 +24,7 @@ function rotateSessionSecret() { return auth.secret; } -/* Keeps an existing secret: rotating here would sign out every device each time - the server needed the key. */ +/* Keep an existing secret. Rotating here signs out every device. */ function getOrCreateSecret() { const cfg = loadConfig(); if (cfg.settings?.auth?.secret) return cfg.settings.auth.secret; @@ -40,14 +35,12 @@ function getOrCreateSecret() { } /* Password hashing, PHC format: $scrypt$ln=14,r=8,p=5$$. - Each hash records the parameters it was made with, which is what makes the - cost adjustable without locking out every existing password. */ + Each hash records the parameters it was made with. */ const SCRYPT_KEYLEN = 64; const SALT_BYTES = 16; -/* Whole rows only, so the parameters cannot be set to an unbalanced pair. The - default suits a 512 MB board; memory is what fails outright on small hardware. */ +/* Whole rows only. The parameters must not be set to an unbalanced pair. */ const HASH_PROFILES = Object.freeze({ '8mib': { ln: 13, r: 8, p: 10 }, '16mib': { ln: 14, r: 8, p: 5 }, @@ -57,7 +50,7 @@ const HASH_PROFILES = Object.freeze({ }); const DEFAULT_PROFILE = '16mib'; -/* scrypt needs roughly 128 * N * r bytes and node:crypto refuses above maxmem, +/* scrypt needs roughly 128 * N * r bytes. node:crypto refuses above maxmem, which defaults to 32 MiB. */ const _maxmemFor = ({ ln, r }) => Math.max(33554432, 128 * 2 ** ln * r * 2); @@ -85,8 +78,7 @@ const _scrypt = (password, salt, params) => ); }); -/** Produce a PHC-format hash using the active profile. - @param {string} password @returns {Promise} */ +/** @param {string} password @returns {Promise} */ async function hashPassword(password) { const params = _activeProfile(); const salt = crypto.randomBytes(SALT_BYTES); @@ -94,21 +86,19 @@ async function hashPassword(password) { return `$scrypt$ln=${params.ln},r=${params.r},p=${params.p}$${_b64(salt)}$${_b64(key)}`; } -/* The pre-PHC format: two hex fields, scrypt with node's defaults. Still - verified, and rewritten on the next successful login. */ +/* The pre-PHC format: two hex fields, scrypt with node's defaults. */ const LEGACY_RE = /^([0-9a-f]+):([0-9a-f]{128})$/i; const LEGACY_PARAMS = Object.freeze({ ln: 14, r: 8, p: 1 }); const PHC_RE = /^\$scrypt\$ln=(\d{1,2}),r=(\d{1,3}),p=(\d{1,3})\$([A-Za-z0-9+/]+)\$([A-Za-z0-9+/]+)$/; -/* A corrupted hash must fail the login, not ask for an allocation that takes the - process down: ln=20 with r=8 is already 1 GiB. */ +/* A corrupted hash must fail the login, not ask for an allocation that takes + the process down. ln=20 with r=8 is already 1 GiB. */ const LN_MAX = 20; const R_MAX = 32; const P_MAX = 64; -/** Parse a stored hash into what is needed to verify it. - @param {unknown} stored +/** @param {unknown} stored @returns {{ params:{ln:number,r:number,p:number}, salt:Buffer, key:Buffer, legacy:boolean }|null} */ function parseHash(stored) { const str = typeof stored === 'string' ? stored : ''; @@ -127,8 +117,8 @@ function parseHash(stored) { const legacy = LEGACY_RE.exec(str); if (legacy) { - /* The salt was fed to scrypt as the hex string itself, not as decoded - bytes, so it has to be passed the same way to reproduce the key. */ + /* The salt was fed to scrypt as the hex string, not as decoded bytes. It + must be passed the same way to reproduce the key. */ return { params: LEGACY_PARAMS, salt: Buffer.from(legacy[1], 'utf8'), @@ -140,15 +130,12 @@ function parseHash(stored) { return null; } -/** True when `password` matches the stored hash. Resolves false for any hash this - function cannot verify; never rejects on a malformed one, and never throws - asynchronously, because a damaged hash must fail the login rather than take - the server down. +/** Must never throw. A damaged hash fails the login rather than taking the + server down. @param {string} password @param {unknown} hash @returns {Promise} */ async function verifyPassword(password, hash) { const parsed = parseHash(hash); if (!parsed) { - /* The effect is a lockout, and the reason is not otherwise visible. */ if (hash) log.error('stored password hash is malformed, login cannot succeed', { reason: 'bad_hash_format' }); return false; } @@ -159,8 +146,7 @@ async function verifyPassword(password, hash) { log.error('password verification failed', { error: e.message }); return false; } - /* timingSafeEqual throws on a length mismatch, where nothing else would catch - it. */ + /* timingSafeEqual throws on a length mismatch. */ try { return crypto.timingSafeEqual(parsed.key, derived); } catch { @@ -168,38 +154,29 @@ async function verifyPassword(password, hash) { } } -/** True when a verified hash should be rewritten: it is in the old format, or it - was made with weaker parameters than the active profile. Callers rewrite it - after a successful login, which is the only moment the password is known. +/** True when a verified hash should be rewritten. Only a successful login can + act on it: that is the one moment the password is known. @param {unknown} hash @returns {boolean} */ function needsRehash(hash) { const parsed = parseHash(hash); - if (!parsed) return false; /* unverifiable; nothing to carry over */ + if (!parsed) return false; if (parsed.legacy) return true; const want = _activeProfile(); const work = ({ ln, r, p }) => 2 ** ln * r * p; return work(parsed.params) < work(want); } -/* 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. - - 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. */ +/* The signed issued-at inside the token is the control. The cookie Max-Age is a + browser hint that must stay in sync with it. This is an idle window: + refreshSession reissues a session that is in use. */ const DEFAULT_MAX_AGE_HOURS = 12; const _maxAgeDays = Number(process.env.SESSION_MAX_AGE_DAYS); 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. */ +/* `${sessionId}.${issuedAt}.${sig}`, where sig covers the first two. The signed + timestamp is what enforces the max age with no session store. */ function makeToken(sessionId, secret) { const iat = Date.now(); const payload = `${sessionId}.${iat}`; @@ -207,10 +184,7 @@ function makeToken(sessionId, secret) { return `${payload}.${sig}`; } -/** 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 +/** @param {string} token @param {string} secret @returns {{ sessionId: string, iat: number }|null} */ function readToken(token, secret) { const dot2 = token.lastIndexOf('.'); @@ -218,7 +192,7 @@ function readToken(token, secret) { const sig = token.slice(dot2 + 1); const rest = token.slice(0, dot2); const dot1 = rest.lastIndexOf('.'); - if (dot1 === -1) return null; /* legacy 2-part tokens (no issued-at) are rejected */ + if (dot1 === -1) return null; /* a 2-part token carries no issued-at */ const sessionId = rest.slice(0, dot1), iat = rest.slice(dot1 + 1); if (sig.length !== 64 || !/^[0-9a-f]+$/.test(sig)) return null; @@ -233,11 +207,11 @@ function verifyToken(token, secret) { return readToken(token, secret)?.sessionId ?? null; } -/* decodeURIComponent throws on an invalid escape, so a stray '%' in any cookie - on the domain, not only ours, would fail every authenticated request. */ +/* Decode without throwing. A stray '%' in any cookie on the domain would + otherwise fail every authenticated request. */ function parseCookies(req) { const header = req.headers.cookie || ''; - /* Null prototype: the keys are cookie names straight off the request. */ + /* Null prototype. The keys are cookie names straight off the request. */ const out = Object.create(null); for (const part of header.split(';')) { const [k, ...v] = part.trim().split('='); @@ -248,8 +222,8 @@ function parseCookies(req) { const TRUST_PROXY = process.env.TRUST_PROXY === 'true'; -/* A Secure cookie set over plain HTTP, which is the normal case on a LAN, is - silently dropped by the browser and breaks login with no visible error. */ +/* A Secure cookie set over plain HTTP, the normal case on a LAN, is dropped by + the browser and breaks login with no visible error. */ function isSecureRequest(req) { if (req.socket?.encrypted) return true; if (TRUST_PROXY) { @@ -270,28 +244,22 @@ function clearSessionCookie(res, secure) { res.setHeader('Set-Cookie', `ds=; HttpOnly;${flag} SameSite=Strict; Path=/; Max-Age=0`); } -/* ── Fixed-window rate limiting ─────────────────────────────────────────────── - - One counter, two surfaces. Login attempts and the polling routes differ only - in what they key on and how they word the wait, so the arithmetic lives here - once and the surfaces stay separate. */ +/* ── Fixed-window rate limiting ─────────────────────────────────────────────── */ /* Count this hit against `key`, and say how much of the window is left if it is refused. - Checking and counting are one synchronous step with no await between, or a - burst of requests all clears the check before any is counted; there is no - exported way to ask without counting. A refused hit is not counted, so a - lockout expires on schedule instead of being extended by what it refuses. + Keep checking and counting one synchronous step. With an await between, a + burst of requests all clears the check before any is counted. Do not export a + way to ask without counting. @param {Map} store @param {string} key @param {number} max @param {number} windowMs @returns {number|null} ms remaining while refused, null when allowed */ function hit(store, key, max, windowMs) { const now = Date.now(); - /* A ceiling below one refuses everything. Worth stating, because the opening - of a fresh window counts the hit that opened it, so without this a limit of - zero would let one request through. */ + /* A fresh window counts the hit that opened it. Without this, a ceiling of + zero lets one request through. */ if (max < 1) return windowMs; const rec = store.get(key); if (!rec || now - rec.first > windowMs) { @@ -307,8 +275,6 @@ const _loginAttempts = new Map(); const LOGIN_MAX = 5, LOGIN_WINDOW_MS = 15 * 60 * 1000; -/* Reported in whole minutes: a fifteen-minute lockout counted down in seconds - reads as a stopwatch on a screen someone is already locked out of. */ function registerLoginAttempt(ip) { const left = hit(_loginAttempts, ip, LOGIN_MAX, LOGIN_WINDOW_MS); if (left === null) return null; @@ -322,27 +288,21 @@ function clearAttempts(ip) { const _rateBuckets = new Map(); -/* Keyed by ip and route, so one client hitting a ceiling on one route does not - affect its own use of another, or anyone else. Reported in seconds: these - windows are a minute, and there is nothing for the caller to do but retry. */ function rateLimit(ip, key, max, windowMs) { const left = hit(_rateBuckets, `${ip}:${key}`, max, windowMs); if (left === null) return null; return `Rate limit exceeded. Try again in ${Math.ceil(left / 1000)}s.`; } -/* A window that has passed is rewritten by the next hit on that key, so this - only clears keys nobody comes back to. */ setInterval(() => { const now = Date.now(); for (const [k, v] of _rateBuckets) if (now - v.first > 3_600_000) _rateBuckets.delete(k); for (const [k, v] of _loginAttempts) if (now - v.first > LOGIN_WINDOW_MS) _loginAttempts.delete(k); }, 600_000).unref(); -/* Auth on with no password stored is an unusable state, not a stricter one: - every login is refused while every route is gated, which locks the install - out. Treating it as off is not a bypass, since with no hash there is no - credential to present and no session to forge. */ +/* Auth on with no password stored locks the install out: every login is refused + while every route is gated. With no hash there is no credential to present and + no session to forge, so treating it as off is not a bypass. */ function authActive(cfg) { const auth = cfg?.settings?.auth; return !!(auth?.enabled && auth?.passwordHash); @@ -358,12 +318,10 @@ 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. */ +/* Extend a session that is still in use. 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. */ function refreshSession(req, res) { const cfg = loadConfig(); if (!authActive(cfg)) return false; @@ -377,8 +335,7 @@ function refreshSession(req, res) { return true; } -/* Unlike isAuthenticated, requires a real session even when auth is off. For - operations such as changing an existing password. */ +/* Unlike isAuthenticated, requires a real session even when auth is off. */ function hasValidSession(req) { const cfg = loadConfig(); const secret = cfg.settings?.auth?.secret; @@ -413,7 +370,6 @@ module.exports = { rateLimit, isAuthenticated, hasValidSession, - /* Tests exercise limits in sequence in one process. */ _resetRateLimits: () => _rateBuckets.clear(), SESSION_MAX_AGE_MS, RENEW_AFTER_MS, diff --git a/api/src/badge-extract.js b/api/src/badge-extract.js index 43e4a73e..e0c6e786 100644 --- a/api/src/badge-extract.js +++ b/api/src/badge-extract.js @@ -1,6 +1,3 @@ -/* Value extraction for activity badges. Plain parsed JSON in, no I/O, so keep it - free of the route it serves. */ - function collectNumbers(obj, path = '', out = [], _depth = 0, _state = { n: 0 }) { const MAX_DEPTH = 6, MAX_NODES = 256; diff --git a/api/src/badge-headers.js b/api/src/badge-headers.js index a1d5da17..91937b30 100644 --- a/api/src/badge-headers.js +++ b/api/src/badge-headers.js @@ -1,30 +1,26 @@ -/* Badge and activity header and query rows: { key, value, secret }. A secret row - follows the same contract as a widget secret field, so its value never leaves - the server and a save that omits it keeps the stored one. */ +/* Badge and activity header and query rows: { key, value, secret }. A secret + row's value never leaves the server. A save that omits it keeps the stored + value. */ const SUBKEYS = ['headers', 'params']; -/** A single well-formed row. @param {unknown} r @returns {boolean} */ +/** @param {unknown} r @returns {boolean} */ function isRow(r) { return ( !!r && typeof r === 'object' && !Array.isArray(r) && typeof (/** @type {{key?:unknown}} */ (r).key) === 'string' ); } -/** Already in the row shape, so the migration has nothing to do. - @param {unknown} v @returns {boolean} */ +/** @param {unknown} v @returns {boolean} */ function isRowArray(v) { return Array.isArray(v) && v.every(isRow); } -/* Old shape ({ key: value }) to rows. An array is always rows, however damaged: - sending one down the legacy-object branch turns its indices into header names - and drops the real credential. Bad elements are skipped rather than refused, - since this runs on stored config; validateRows rejects them on the way in. */ +/* Old shape ({ key: value }) to rows. Treat an array as rows however damaged. + The legacy-object branch turns array indices into header names and drops the + real credential. */ function toRows(v) { if (Array.isArray(v)) { - /* Returned as-is when nothing needs dropping, so the common path neither - allocates nor breaks callers that rely on getting the same array back. */ return v.every(isRow) ? v : v.filter(isRow); } if (v && typeof v === 'object') { @@ -33,16 +29,12 @@ function toRows(v) { return []; } -/** How many entries toRows would discard. Lets a caller report the damage - rather than silently working with less than it was given. - @param {unknown} v @returns {number} */ +/** @param {unknown} v @returns {number} */ function droppedRowCount(v) { return Array.isArray(v) ? v.length - v.filter(isRow).length : 0; } -/** The first badge or activity row on an item that is not well-formed, or null. - Used to reject a save naming the field, so damaged rows cannot be stored. - @param {unknown} item @returns {{ field:string, index:number }|null} */ +/** @param {unknown} item @returns {{ field:string, index:number }|null} */ function firstMalformedRow(item) { if (!item || typeof item !== 'object') return null; const it = /** @type {{ badge?: any, monitoring?: { activity?: any } }} */ (item); @@ -53,7 +45,7 @@ function firstMalformedRow(item) { if (!block || typeof block !== 'object') continue; for (const sub of SUBKEYS) { const v = block[sub]; - if (!Array.isArray(v)) continue; /* the legacy object shape is still accepted */ + if (!Array.isArray(v)) continue; const at = v.findIndex(r => !isRow(r)); if (at !== -1) return { field: `${label}.${sub}`, index: at }; } @@ -61,26 +53,17 @@ function firstMalformedRow(item) { return null; } -/* Skips a blank key or a null value, which is a scrubbed secret with nothing - stored. - @param {any} rows - @returns {Record} */ +/** @param {any} rows @returns {Record} */ function rowsToObject(rows) { /** @type {Record} */ const out = Object.create(null); for (const r of toRows(rows)) { - /* An empty value is not a header worth sending. It happens when a stored - credential was withheld because the request no longer matches the one it - was saved for, and sending the name with nothing behind it only tells the - far end that the field exists. */ if (!r.key || r.value == null || r.value === '') continue; out[r.key] = r.value; } return out; } -/* The activity block wins over the badge block when both exist, matching - badges.js. Returns { headers, params } as plain objects for a fetch. */ function requestParts(item) { const src = item?.monitoring?.activity?.enabled ? item.monitoring.activity : item?.badge; return { @@ -96,19 +79,14 @@ function scrubRows(rows) { }); } -/* Refills a secret row the browser sent without its value, matching by key. - - A row arriving as non-secret is never refilled, even when a value is stored - for that key: it would move the credential into a row that scrubRows sends to - the browser in full. Unticking Secret therefore clears the stored value. See - docs/security.md. */ +/* Never refill a row that arrives non-secret. It would move the credential into + a row scrubRows sends to the browser in full. */ function preserveRows(newRows, oldRows) { const nrows = toRows(newRows); const orows = toRows(oldRows); for (const r of nrows) { const needsValue = r.value == null || r.value === ''; if (needsValue) { - /* Only a row still marked secret may be refilled. */ const donor = r.secret ? orows.find(o => o.key === r.key && o.value != null && o.value !== '') : null; if (donor) r.value = donor.value; else if (!r.secret) r.value = ''; @@ -139,8 +117,6 @@ function preserveItemBadgeSecrets(newItem, oldItem) { if (newItem?.monitoring?.activity) apply(newItem.monitoring.activity, oldBlocks.activity); } -/* Existing entries default to secret:false, which keeps current behaviour: their - sensitivity is unknown. */ function migrateItemBadgeHeaders(item) { let changed = false; eachActivityLike(item, block => { diff --git a/api/src/config-secrets.js b/api/src/config-secrets.js index 7cc85752..143d4812 100644 --- a/api/src/config-secrets.js +++ b/api/src/config-secrets.js @@ -1,6 +1,5 @@ -/* Every item-level secret system in one place, so a route handling a full config - cannot run some of them and miss others. Settings-level secrets are not item - secrets and stay in the config route. */ +/* Every item-level secret system in one place. A route that runs some and + misses others leaks a stored credential. */ const { scrubConfigSecrets, preserveConfigSecrets, secretSpec } = require('./widget-secrets'); const { scrubItemBadgeSecrets, preserveItemBadgeSecrets, toRows } = require('./badge-headers'); @@ -15,8 +14,6 @@ function scrubAllSecrets(cfg) { return cfg; } -/* Whether anything would actually be refilled from this block, so an item is - only reported as needing a credential when one was really withheld. */ function rowsHoldSecret(rows) { return toRows(rows).some(r => r.secret && r.value != null && r.value !== ''); } @@ -25,8 +22,6 @@ function blockHoldsSecret(block) { return !!block && (rowsHoldSecret(block.headers) || rowsHoldSecret(block.params)); } -/* The saved request a block would be refilled against, in the shape the scope - check compares. */ const asRequest = block => ({ url: block?.url, headers: block?.headers, params: block?.params }); function widgetHoldsSecret(config, entry) { @@ -44,18 +39,8 @@ function widgetHoldsSecret(config, entry) { } /** Refill the secrets a save arrived without, but only for the items whose - request is still the one the secret was stored for. - - A saved config names where each credential is sent. Matching a stored secret - to an incoming item by its id alone would refill it for a request that now - points somewhere else, and the next poll would deliver the credential there: - a config file is something people import from elsewhere, so that is a way to - take a token out of an install by sending its owner a file. The same check - already guards the two endpoints that test a badge or a widget; this is the - save path using it too. - - Returns the items whose secret was withheld, so the caller can say which - credentials have to be entered again. + request is still the one the secret was stored for. Matching by item id + alone sends a stored credential to a destination the incoming config names. @param {any} newCfg @param {any} oldCfg @returns {{ withheld: Array<{ id: string, label: string }> }} */ @@ -68,8 +53,6 @@ function preserveAllSecrets(newCfg, oldCfg) { const prevOf = item => oldItems.find(e => e && e.id === item.id); if (Array.isArray(newCfg.items)) { - /* Widgets first: preserveConfigSecrets walks the whole config, so the items - it must not refill are the ones whose config no longer matches. */ const reg = getRegistry(); const scopedForWidgets = { ...newCfg, items: [] }; const keepWidget = new Map(); @@ -81,8 +64,8 @@ function preserveAllSecrets(newCfg, oldCfg) { keepWidget.set(item, ok); if (!ok && widgetHoldsSecret(prev.widgetConfig, entry)) note(item); } - /* preserveConfigSecrets reads oldCfg.items by id, so an item that must not - be refilled is hidden from it rather than handled separately. */ + /* preserveConfigSecrets matches oldCfg.items by id. Hide an item from it to + stop its secret being refilled. */ scopedForWidgets.items = newCfg.items; const oldForWidgets = { ...oldCfg, @@ -100,9 +83,7 @@ function preserveAllSecrets(newCfg, oldCfg) { preserveItemBadgeSecrets(item, undefined); continue; } - /* Each block carries its own destination, so they are judged separately: - changing where the activity badge points must not also drop the - credential the status badge uses. */ + /* Judge each block on its own. They carry separate destinations. */ const scoped = { ...prev }; for (const [block, oldBlock, set] of [ [item.badge, prev.badge, b => (scoped.badge = b)], diff --git a/api/src/config.js b/api/src/config.js index 0a46da57..b87324f8 100644 --- a/api/src/config.js +++ b/api/src/config.js @@ -11,15 +11,9 @@ let _cfgCache = null, _cfgCacheAt = 0; const CONFIG_TTL_MS = 5000; -/* Bump when a release changes the shape, and add a matching step in migrate(). */ +/* Bump when a release changes the shape. Add a matching step in migrate(). */ const SCHEMA_VERSION = 3; -/* Admin suggested `tcp://socket-proxy:2375` for the Docker socket, and a URL - with an unrecognised scheme used to be sent as an HTTP request regardless. - Outbound requests are now restricted to http and https, so a stored tcp URL - is refused, the container list comes back empty, and every app backed by a - container reports unhealthy at once. The address is the same host and port - either way, so it is rewritten rather than cleared. */ function migrateSocketProxyScheme(settings) { const url = settings?.server?.socketProxyUrl; if (typeof url !== 'string' || !/^tcp:\/\//i.test(url)) return false; @@ -27,8 +21,8 @@ function migrateSocketProxyScheme(settings) { return true; } -/* Idempotent, so it is safe on every read and write. A config with no - _schemaVersion is version 1. Add ordered steps: `if (v < 2) { ...; v = 2; }`. */ +/* Must stay idempotent. It runs on every read and every write. A config with no + _schemaVersion is version 1. */ function migrate(cfg) { if (!cfg || typeof cfg !== 'object') return cfg; let v = Number(cfg._schemaVersion) || 1; @@ -48,8 +42,7 @@ function migrate(cfg) { } let _demoCfg = null; -/* Read from the bundled showcase file, never from disk, so nothing a visitor - does can persist. */ +/* Never read from disk. Nothing a demo visitor does may persist. */ function loadDemoConfig() { if (!_demoCfg) { const raw = fs.readFileSync(path.join(__dirname, '..', 'demo', 'demo-config.json'), 'utf8'); @@ -59,9 +52,6 @@ function loadDemoConfig() { return _demoCfg; } -/* items is what every consumer iterates, so a wrong-typed one is the crash - vector. A missing items or settings is repaired rather than rejected, so a - minimal but valid config is kept instead of being backed up and blanked. */ function _normalizeShape(parsed) { if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) return null; if (parsed.items !== undefined && !Array.isArray(parsed.items)) return null; @@ -70,8 +60,7 @@ function _normalizeShape(parsed) { return parsed; } -/* Timestamped and written with wx, so one corruption cannot overwrite an earlier - backup, and the same content is preserved only once. */ +/* Written with wx. One corruption must not overwrite an earlier backup. */ let _lastCorruptRaw = null; function _backupCorrupt(raw) { if (raw === _lastCorruptRaw) return; @@ -101,7 +90,6 @@ function loadConfig() { try { parsed = JSON.parse(raw); } catch (e) { - /* Preserve it instead of letting the next save overwrite it. */ log.warn('config file corrupt, backing up and starting with a blank config', { path: CONFIG_PATH, error: e.message, @@ -112,8 +100,6 @@ function loadConfig() { const shaped = _normalizeShape(parsed); if (!shaped) { - /* Valid JSON, wrong shape. Treated like an unparseable file rather than - caching a shape that throws for every consumer. */ log.warn('config file has the wrong shape, backing up and starting with a blank config', { path: CONFIG_PATH }); _backupCorrupt(raw); return migrate({ items: [], settings: {} }); @@ -123,8 +109,7 @@ function loadConfig() { migrate(shaped); _cfgCache = shaped; _cfgCacheAt = now; - /* A failed write, on a read-only volume for instance, must not break reads: the - migrated copy is cached and re-migrates next load. */ + /* A failed write must not break reads. */ if (shaped._schemaVersion !== before) { try { saveConfig(shaped); @@ -133,8 +118,6 @@ function loadConfig() { return shaped; } -/* Every write bumps _rev, and POST /api/config rejects a stale one, so two admin - tabs saving over each other is a 409 rather than a silent loss. */ function saveConfig(data) { if (data && typeof data === 'object') { data._schemaVersion = SCHEMA_VERSION; @@ -144,11 +127,8 @@ function saveConfig(data) { fs.mkdirSync(dir, { recursive: true }); const tmp = CONFIG_PATH + '.tmp'; - /* Write, flush, rename, flush the directory. Temp-and-rename alone only stops a - reader seeing a half-written file; without the flushes a power cut can leave - the rename applied and the contents lost, which is a real case on a Pi that - gets unplugged. The second flush is on the directory, because the rename is - a directory entry. */ + /* Write, flush, rename, flush the directory. Without both flushes a power cut + leaves the rename applied and the contents lost. */ let fd; try { fd = fs.openSync(tmp, 'w'); @@ -161,7 +141,6 @@ function saveConfig(data) { try { fs.renameSync(tmp, CONFIG_PATH); } catch (e) { - /* Otherwise a failed save leaves a temp file behind for good. */ try { fs.unlinkSync(tmp); } catch { @@ -179,17 +158,14 @@ function saveConfig(data) { } } catch { /* Some filesystems refuse to fsync a directory. The contents are already - durable here, so this is not worth failing the save over. */ + durable. */ } - /* Only after the write succeeded, or the app shows changes that were never - saved. */ + /* Only after the write succeeded. */ _cfgCache = data; _cfgCacheAt = Date.now(); } -/* A permanent default item: movable and hideable like any app, but never removed - or edited. Guaranteed present on every read and write. */ const SYSTEM_SETTINGS_ITEM = { id: 'settings', type: 'app', diff --git a/api/src/demo-data.js b/api/src/demo-data.js index 1093f069..ade9ea68 100644 --- a/api/src/demo-data.js +++ b/api/src/demo-data.js @@ -1,7 +1,3 @@ -/* Demo data that is not tied to one widget. A widget's own invented body lives - in its folder, in demo.js. */ - -/* Smooth wave in [min,max], period seconds, plus a little noise. */ function wave(periodSec, min, max, phase = 0) { const t = Date.now() / 1000; const mid = (min + max) / 2, diff --git a/api/src/demo.js b/api/src/demo.js index 505c8412..7c890846 100644 --- a/api/src/demo.js +++ b/api/src/demo.js @@ -1,5 +1,3 @@ -/* With DEMO_MODE=true the instance serves the bundled config in api/demo/, - refuses every write, and makes no outbound requests. */ const IS_DEMO = process.env.DEMO_MODE === 'true'; const DEMO_READONLY_MSG = 'Saving is disabled in the live demo.'; diff --git a/api/src/icon-sniff.js b/api/src/icon-sniff.js index 16de70ef..41d44cdd 100644 --- a/api/src/icon-sniff.js +++ b/api/src/icon-sniff.js @@ -1,8 +1,3 @@ -/* Content check for uploaded raster icons. SVGs go through svg-sanitize instead, - which is a stronger control than a signature match. The goal is to reject a - non-image with an image extension, not to police filename honesty, so a PNG - named .ico passes. */ - const PNG = [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]; /* reserved=0, type=1 (icon). type=2 is a cursor and is not accepted. */ const ICO = [0x00, 0x00, 0x01, 0x00]; diff --git a/api/src/log.js b/api/src/log.js index 0d0d04ed..b9e55902 100644 --- a/api/src/log.js +++ b/api/src/log.js @@ -1,8 +1,4 @@ -/* logfmt logger: "