diff --git a/api/src/auth.js b/api/src/auth.js index 6bec70f5..e93059d5 100644 --- a/api/src/auth.js +++ b/api/src/auth.js @@ -251,25 +251,17 @@ function clearSessionCookie(res, secure) { /* ── Fixed-window rate limiting ─────────────────────────────────────────────── - One counter, two surfaces. Login attempts and the polling routes count the - same way and differ only in what they key on, how they word the wait, and - whether success clears the count, so the arithmetic lives here once. Two - copies of a timing rule is how a pair of them comes to disagree. - - The surfaces stay separate on purpose: one is on the path that can lock - someone out of their own dashboard, and a reader of either does not want the - other's wording in front of them. */ + 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. */ /* 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, so a - burst of concurrent requests cannot all clear the check before any of them is - counted. Splitting this into a read and a write is the check-then-increment - race, which is why there is no exported way to ask without counting. - - A refused hit is not counted. That is what lets a lockout expire on schedule - rather than being extended by the attempts it is refusing. + 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. @param {Map} store @param {string} key @param {number} max @param {number} windowMs diff --git a/api/src/routes/badges.js b/api/src/routes/badges.js index 32b8a2f9..d55f0c6d 100644 --- a/api/src/routes/badges.js +++ b/api/src/routes/badges.js @@ -92,14 +92,12 @@ on('POST', '/api/badge-proxy', async (req, res) => { const body = JSON.parse(await readBody(req)); const { url, itemId, skipTls = false } = body; if (!url) return json(res, 400, { error: 'url required', kind: KIND.INVALID }); - /* Rows the user did not retype arrive as secret rows without a value. Fill - them from the stored item so a test after reload uses the real credential, - without ever sending it to the browser. + /* Rows the user did not retype arrive without a value, and are filled from + the stored item so a test after reload uses the real credential. Only when the request targets exactly the saved destination with exactly - the saved non-secret rows. Otherwise the request picks the URL while the - server picks the credential, and a stored secret would be delivered - wherever the caller asked. See secret-scope.js. */ + the saved non-secret rows, or the caller picks the URL while the server + picks the credential. See secret-scope.js. */ let headerRows = toRows(body.headers); let paramRows = toRows(body.params); let declined = false; diff --git a/api/src/routes/config.js b/api/src/routes/config.js index 08db9ec0..b18930a3 100644 --- a/api/src/routes/config.js +++ b/api/src/routes/config.js @@ -132,15 +132,9 @@ on('POST', '/api/config', async (req, res) => { data.settings.background = data.settings.background || {}; data.settings.background.apiKey = existing.settings.background.apiKey; } - /* settings.auth is owned entirely by the /api/auth/* routes, which is where - the admin UI changes it. Nothing a config write supplies is kept. - - Merging field by field, as this did, left a gap: the merge only ran when - auth already existed, so before a password was ever set an unauthenticated - caller could write settings.auth outright. A hand-made passwordHash then - locked the install, or crashed it, since verifyPassword did not validate - what it was given. Taking the block wholesale closes both, and closes any - field added to it later without this line being revisited. */ + /* settings.auth is owned entirely by the /api/auth/* routes; nothing a + config write supplies is kept. Taking the whole block rather than merging + field by field also covers any field added to it later. */ if (data.settings) delete data.settings.auth; if (existing.settings?.auth) { data.settings = data.settings || {}; diff --git a/api/src/routes/icons.js b/api/src/routes/icons.js index 896ac5b5..aff16e74 100644 --- a/api/src/routes/icons.js +++ b/api/src/routes/icons.js @@ -11,22 +11,12 @@ const { fail, KIND, errorBody } = require('../api-error'); limit is the larger of the two because the request also contains boundaries and headers, so cutting it off at exactly the file limit would reject a file that is within it. */ -/* A safe, unused filename for an upload. +/* A safe, unused filename for an upload. Reusing the submitted name overwrote + an existing icon that other apps still referenced, so a free name is found + instead and returned in the response. - Two problems with using the submitted name directly. - - It overwrote silently. Uploading logo.svg twice replaced the first, and the - replaced icon is still referenced by whichever apps use it, so a tile's - picture changed without anyone touching that app. Nothing warned, and there - was no way back. Refusing the upload instead would punish a common and - innocent case, since a great many icons are called logo.svg; a free name is - found instead, and the response already returns the filename so the form shows - what was actually saved. - - The name is also tidied. path.basename strips a Unix path, but on Linux it - does not treat a backslash as a separator, so a Windows-style name arrived as - the literal "..\..\etc\passwd.svg". That cannot escape the icons directory, - which is what matters, but it makes for a confusing file to find later. + Backslashes are stripped as well as slashes: path.basename does not treat one + as a separator on Linux. @param {string} dir @param {string} raw @returns {string} */ function safeIconName(dir, raw) { diff --git a/api/src/routes/system.js b/api/src/routes/system.js index fcfb23f0..cbdd2c11 100644 --- a/api/src/routes/system.js +++ b/api/src/routes/system.js @@ -9,20 +9,10 @@ let _netPrev = null; /* One /proc/net/dev line, as bytes received and transmitted. - The name is separated from its counters by a colon, not by whitespace, and the - kernel pads the name to a fixed width. Once the receive counter is wide enough - the value runs into the colon: - - eth0: 1234567 890 ... -> fields line up - eth0:123456789012 890 ... -> every field shifts by one - - Splitting on whitespace therefore read packets where it meant bytes, off by - roughly a thousand, and that is the normal case rather than an edge case: it - happens once an interface has carried about 10 MB. Splitting on the colon - first removes the ambiguity. - - The name is matched exactly, too. `startsWith` meant a request for eth0 could - match eth0.100, and a request for eth could match eth0. */ + Split on the colon, not on whitespace: the kernel pads the name to a fixed + width, so once the receive counter is wide enough it runs into the colon and + every field shifts by one. The interface name is matched exactly, or eth0 + also matches eth0.100. */ const RX_BYTES = 0; /* field order after the colon, per the header line */ const TX_BYTES = 8; diff --git a/api/src/secret-scope.js b/api/src/secret-scope.js index b0a66876..e29a1826 100644 --- a/api/src/secret-scope.js +++ b/api/src/secret-scope.js @@ -1,15 +1,10 @@ /* When a stored secret may be attached to a request the caller composed. Two endpoints let the browser post an in-progress config and have the server - fill in a credential. Both let the request choose the destination, so - restoring on the item id alone turns "can edit config" into "can read every - stored credential": point the URL elsewhere, omit the secret, and the server - sends the real one there. - - The rule is deliberately blunt: restore only when every non-secret field is - identical to what is saved. Comparing only the fields that look like a - destination needs an assumption about which fields matter, which is the - assumption that failed here. */ + fill in a credential, and both let the request choose the destination, so + matching on the item id alone would send a stored credential anywhere the + caller names. Restore only when every non-secret field is identical to what + is saved. */ const { secretSpec } = require('./widget-secrets'); const { toRows } = require('./badge-headers'); diff --git a/api/src/widget-i18n.js b/api/src/widget-i18n.js index bfa00f65..8fd2d9f9 100644 Binary files a/api/src/widget-i18n.js and b/api/src/widget-i18n.js differ diff --git a/scripts/bump-cache-busting.js b/scripts/bump-cache-busting.js index 227b884f..0120c1d8 100644 --- a/scripts/bump-cache-busting.js +++ b/scripts/bump-cache-busting.js @@ -4,15 +4,10 @@ const path = require('path'); const crypto = require('crypto'); const UI_DIR = path.join(__dirname, '..', 'ui'); -/* An asset reference, with or without a version stamp. - - The stamp used to be required by this pattern, so a reference written without - one was invisible to this script and stayed unstamped forever. A browser then - kept serving that file from cache after an upgrade while every other file was - refreshed, leaving one page running mixed versions. - - The stamp is optional here, so a new reference is picked up the first time - this runs rather than being silently skipped. */ +/* An asset reference, with or without a version stamp. The stamp is optional + on purpose: requiring it made a reference written without one invisible to + this script, so it stayed unstamped and that one file kept being served from + cache after an upgrade. */ const REF_RE = /(["'])(\/(?:css|js)\/[a-zA-Z0-9_.-]+\.(?:css|js))(\?v=[0-9a-zA-Z]+)?/g; function listFiles(dir, exts, out = []) { diff --git a/scripts/check-frontend-paths.js b/scripts/check-frontend-paths.js index c7c67feb..7c2c0f6e 100644 --- a/scripts/check-frontend-paths.js +++ b/scripts/check-frontend-paths.js @@ -1,17 +1,11 @@ #!/usr/bin/env node -/* Every module under ui/js needs two entries in tsconfig.frontend.json: the - plain path and the cache-busted one. +/* Every module under ui/js needs two entries in tsconfig.frontend.json, the + plain path and the cache-busted one, because TypeScript allows one wildcard + per pattern (TS5061). - TypeScript allows at most one wildcard per pattern (TS5061), so "/js/foo.js" - and "/js/foo.js?v=*" cannot be globbed into a single entry. The frontend - typecheck is the only thing that resolves those specifiers back to real - files, and a module missing from the map is not a loud failure: the import - resolves to nothing, that module goes unchecked, and typecheck:ui still - passes. The gap only shows up as a bug nobody caught. - - The file's own header tells an author to add both. This turns remembering - into a failing check, which is the point: the alternative is a build step, - and this project does not have one. */ + A missing entry fails silently: the import resolves to nothing, that module + goes unchecked, and typecheck:ui still passes. This makes it a failing check + instead. */ const fs = require('node:fs'); const path = require('node:path'); diff --git a/scripts/is-prerelease.js b/scripts/is-prerelease.js index 783e1e0c..4326a828 100644 --- a/scripts/is-prerelease.js +++ b/scripts/is-prerelease.js @@ -1,25 +1,10 @@ #!/usr/bin/env node -/* Does this tag name a stable release, or a candidate? +/* Does this tag name a stable release, or a candidate? The answer decides + whether the build moves `latest`, which is what an unpinned pull gets. - The answer decides whether the build moves the `latest` tag, which is what an - unpinned `docker pull` gets. Getting it wrong in one direction ships a release - candidate to everyone who never pinned a version; in the other it leaves - `latest` behind on a real release. - - The workflow used to ask whether the ref contained a hyphen. That is right - for the tags this project actually cuts, and wrong in two ways that matter: - - v1.5.0+2026-08-08 build metadata, a stable release, but it has a hyphen - v1.5 not a version at all, no hyphen, so it moved `latest` - - The second is the dangerous one. A mistyped tag should never be treated as a - stable release, so anything that is not a valid semver version is reported as - a prerelease: refusing to move `latest` is the safe failure. - - Deliberately not api/src/semver.js. That module compares versions for the - update check and is forgiving by design, coercing a malformed part to 0 so a - strange version from GitHub cannot break the dashboard. Here, malformed input - is the case that has to be caught rather than smoothed over. */ + Anything that is not valid semver is reported as a prerelease, so a mistyped + tag such as `v1.5` cannot move `latest`. Not api/src/semver.js: that one + coerces malformed input rather than rejecting it. */ /* semver.org's own recommended pattern, with an optional leading v for git tag style. Note what it rejects: leading zeroes in the core, an empty prerelease diff --git a/ui/js/admin-logic.js b/ui/js/admin-logic.js index 1fbfb215..64f827da 100644 --- a/ui/js/admin-logic.js +++ b/ui/js/admin-logic.js @@ -129,10 +129,9 @@ export function widgetConfigMode(type, reg) { /** Which admin section to show, given a requested id and the sections present. - Exactly one section must always show. The requested id can be stale, since it - comes from localStorage and can name a section an older version had, and - falling through leaves the page blank. Falls back to the first available - section rather than a hard-coded name. + Exactly one section must always show, and the requested id comes from + localStorage so it can name a section an older version had. Falls back to the + first available section rather than a hard-coded name. @param {string|null|undefined} requested @param {string[]} available in document order @@ -191,17 +190,11 @@ export function reorderItems(items, item, dir, { folderId = null, childIdx = nul return true; } -/* The reasons a widget was refused, one line per reason, ready to be written as - text. Kept here rather than in the form so the two places that show refusals - cannot word them differently, and so the shape coming off the API can be - checked without a DOM. - - The shape is request-adjacent: it is built by the server from files on disk, - but it reaches this code as JSON, so an entry that is not a named widget with - at least one string reason is dropped rather than rendered as "undefined". - - `withName` distinguishes the two callers: the editor is already showing one - widget, the picker is listing several. */ +/* The reasons a widget was refused, one line per reason, so the two places that + show refusals cannot word them differently. The shape arrives as JSON, so an + entry that is not a named widget with at least one string reason is dropped + rather than rendered as "undefined". `withName` separates the two callers: + the editor shows one widget, the picker lists several. */ export function rejectionLines(rejections, { withName = true } = {}) { if (!Array.isArray(rejections)) return []; const out = []; diff --git a/ui/js/dashboard-logic.js b/ui/js/dashboard-logic.js index e55e6717..60399c4f 100644 --- a/ui/js/dashboard-logic.js +++ b/ui/js/dashboard-logic.js @@ -24,13 +24,9 @@ function fingerprint(c) { /** Where to send the browser once first-run setup is finished. - A fresh install has no items, and the dashboard is where items are shown, so - landing there shows an empty page with nothing to act on. Admin is where the - first item gets added, and is the page the user would have to open anyway. - - Only the automatic step after setup. Opening `/` afterwards is not blocked, - and an empty dashboard reached deliberately stays empty: there are no - placeholder items to delete. + A fresh install has no items, so the dashboard has nothing to act on and + Admin is where the first one is added. Only the automatic step after setup: + opening `/` afterwards is not blocked, and an empty dashboard stays empty. @param {unknown} items the dashboard items the page was built from @returns {string|null} a path to navigate to, or null to stay put */ diff --git a/ui/js/link-url.js b/ui/js/link-url.js index 8a073329..7bff8da6 100644 --- a/ui/js/link-url.js +++ b/ui/js/link-url.js @@ -1,11 +1,9 @@ /* Which URLs are safe to put in a link the user can click. - A denylist on purpose: a tile link is handed to the OS, and a homelab - dashboard legitimately links to ssh://, vnc://, rdp:// and whatever else the - user has registered. Only the schemes that execute script in our own origin - are refused, and rel="noopener noreferrer" does nothing about those. - - Enforced on save and again on render, since a config can arrive by import. + A denylist on purpose: a tile link is handed to the OS, and a homelab links + to ssh://, vnc:// and whatever else the user registered, so only the schemes + that execute script in our own origin are refused. Enforced on save and again + on render, since a config can arrive by import. The server requires this file directly, so it must stay free of anything only a browser has: no DOM, no window, no imports. */