diff --git a/dashcaddy-api/__tests__/state-manager.test.js b/dashcaddy-api/__tests__/state-manager.test.js index 1ec428d..e7fe590 100644 --- a/dashcaddy-api/__tests__/state-manager.test.js +++ b/dashcaddy-api/__tests__/state-manager.test.js @@ -6,6 +6,9 @@ jest.mock('fs', () => ({ promises: { readFile: jest.fn().mockResolvedValue('[]'), writeFile: jest.fn().mockResolvedValue(), + // DC-170 atomic writes: _atomicWrite serializes to a tmp file then renames + rename: jest.fn().mockResolvedValue(), + unlink: jest.fn().mockResolvedValue(), }, })); @@ -74,18 +77,23 @@ describe('StateManager', () => { }); describe('write', () => { - it('acquires lock, writes JSON, releases lock', async () => { + it('acquires lock, writes JSON atomically (tmp+rename), releases lock', async () => { const releaseFn = jest.fn().mockResolvedValue(); lockfile.lock.mockResolvedValue(releaseFn); await sm.write([{ id: 'new' }]); expect(lockfile.lock).toHaveBeenCalledWith(TEST_PATH, expect.any(Object)); + // DC-170 atomic contract: tmp file write + rename over the live file expect(fs.promises.writeFile).toHaveBeenCalledWith( - TEST_PATH, + expect.stringContaining(`${TEST_PATH}.tmp-`), JSON.stringify([{ id: 'new' }], null, 2), 'utf8' ); + expect(fs.promises.rename).toHaveBeenCalledWith( + expect.stringContaining(`${TEST_PATH}.tmp-`), + TEST_PATH + ); expect(releaseFn).toHaveBeenCalled(); }); diff --git a/dashcaddy-api/routes/calendar.js b/dashcaddy-api/routes/calendar.js index 7bed812..312f9b5 100644 --- a/dashcaddy-api/routes/calendar.js +++ b/dashcaddy-api/routes/calendar.js @@ -142,8 +142,21 @@ module.exports = function createCalendarRoutes({ if (req.body.user !== undefined) updates.user = req.body.user || null; if (updates.name !== undefined) updates.name = String(updates.name).slice(0, 120); if (req.body.pass !== undefined || req.body.token !== undefined) { + // DC-170: preserve the credential TYPE not being updated — the old write + // stored {pass: body.pass ?? null, token: body.token ?? null}, so + // updating pass on a token-authenticated source (or vice versa) nulled + // the other field and broke the source's stored credential. + let existingCreds = {}; + try { + const raw = await credentialManager.retrieve(`calendar:source:${existing.id}`); + if (raw) existingCreds = JSON.parse(raw); + } catch (_) { /* start from empty */ } + const next = { + pass: req.body.pass !== undefined ? req.body.pass : (existingCreds.pass ?? null), + token: req.body.token !== undefined ? req.body.token : (existingCreds.token ?? null), + }; await credentialManager.store(`calendar:source:${existing.id}`, - JSON.stringify({ pass: req.body.pass ?? null, token: req.body.token ?? null }), + JSON.stringify(next), { label: `Calendar source ${existing.name}`, updatedAt: new Date().toISOString() }); } await calendarStateManager.updateItem(existing.id, updates); @@ -174,7 +187,9 @@ module.exports = function createCalendarRoutes({ const results = await Promise.allSettled(wanted.map(async (source) => { if (source.type === 'ics') { - const resp = await fetchT(source.url, { timeout: 15000 }); + // DC-170: fetchT ignores opts.timeout (undici) — the 15s intent ran at + // the 5s HTTP_DEFAULT. Pass it as the third arg (timeoutMs). + const resp = await fetchT(source.url, {}, 15000); if (!resp.ok) throw new Error(`ICS fetch HTTP ${resp.status}`); // Bounded read (judge r3 blocker): same 5 MiB cap as the CalDAV path — // a misbehaving feed must not balloon API memory. diff --git a/dashcaddy-api/routes/config/settings.js b/dashcaddy-api/routes/config/settings.js index 9cb6d4e..2924718 100644 --- a/dashcaddy-api/routes/config/settings.js +++ b/dashcaddy-api/routes/config/settings.js @@ -28,7 +28,15 @@ module.exports = function({ configStateManager: _configStateManager, asyncHandle return res.json({ setupComplete: false }); } const data = await fsp.readFile(ctx.CONFIG_FILE, 'utf8'); - const config = JSON.parse(data); + let config; + try { + config = JSON.parse(data); + } catch (_) { + // DC-170: a corrupt/partial config.json (e.g. non-atomic write crash) + // previously 500'd this PUBLIC endpoint on every dashboard load. + // Degrade to the setup wizard instead of an error page. + return res.json({ setupComplete: false, corrupt: true }); + } res.json(config); }, 'config-get')); diff --git a/dashcaddy-api/routes/dns.js b/dashcaddy-api/routes/dns.js index e41aa1a..73c2bf8 100644 --- a/dashcaddy-api/routes/dns.js +++ b/dashcaddy-api/routes/dns.js @@ -718,11 +718,17 @@ module.exports = function({ const text = await response.text(); + // DC-170: every path must answer — bare returns left requests hanging if (!text || text.trim() === '') { - return // Error handled by middleware + return errorResponse(res, 'Empty response from DNS server update check', 502); } - const result = JSON.parse(text); + let result; + try { + result = JSON.parse(text); + } catch (_) { + return errorResponse(res, 'DNS server returned a non-JSON update response', 502); + } if (result.status === 'ok') { success(res, { @@ -735,11 +741,11 @@ module.exports = function({ instructionsLink: result.response.instructionsLink || null }); } else { - // Error handled by middleware + return errorResponse(res, `DNS server update check failed: ${result.message || result.status}`, 502); } } catch (error) { log.error('dns', error, null, { note: 'DNS update check error' }); - // Error handled by middleware + return errorResponse(res, safeErrorMessage(error), error.statusCode || 502); } }, 'dns-check-update')); @@ -776,12 +782,17 @@ module.exports = function({ const checkText = await checkResponse.text(); if (!checkText || checkText.trim() === '') { - return // Error handled by middleware + return errorResponse(res, 'Empty response from DNS server update check', 502); + } + let checkResult; + try { + checkResult = JSON.parse(checkText); + } catch (_) { + return errorResponse(res, 'DNS server returned a non-JSON update response', 502); } - const checkResult = JSON.parse(checkText); if (checkResult.status !== 'ok') { - return // Error handled by middleware + return errorResponse(res, `DNS server update check failed: ${checkResult.message || checkResult.status}`, 502); } if (!checkResult.response.updateAvailable) { @@ -807,7 +818,7 @@ module.exports = function({ }); } catch (error) { log.error('dns', error, null, { note: 'DNS update error' }); - // Error handled by middleware + return errorResponse(res, safeErrorMessage(error), error.statusCode || 502); } }, 'dns-update')); diff --git a/dashcaddy-api/src/managers/state-manager.js b/dashcaddy-api/src/managers/state-manager.js index ed963ca..a08d2e6 100644 --- a/dashcaddy-api/src/managers/state-manager.js +++ b/dashcaddy-api/src/managers/state-manager.js @@ -36,6 +36,24 @@ class StateManager { this._ensureFileExists(); } + /** + * Atomic write (DC-170): serialize to a temp file in the same directory, + * then rename over the live file. rename(2) is atomic on POSIX, so a crash + * mid-write can no longer leave a truncated JSON file behind (the old + * truncate-then-write corrupted services.json/config.json unrecoverably — + * read() only special-cases ENOENT). Caller must hold the file lock. + */ + async _atomicWrite(data) { + const tmpPath = `${this.filePath}.tmp-${process.pid}-${Date.now()}`; + await fs.writeFile(tmpPath, JSON.stringify(data, null, 2), 'utf8'); + try { + await fs.rename(tmpPath, this.filePath); + } catch (err) { + try { await fs.unlink(tmpPath); } catch (_) { /* best effort */ } + throw err; + } + } + /** * Ensure the state file exists, create with empty array if not * @private @@ -81,8 +99,10 @@ class StateManager { // Acquire lock release = await lockfile.lock(this.filePath, this.lockOptions); - // Write data with pretty formatting - await fs.writeFile(this.filePath, JSON.stringify(data, null, 2), 'utf8'); + // Write data atomically (DC-170): truncate-then-write on the live file + // corrupted services.json/config.json on crash mid-write with no recovery + // path (read() only special-cases ENOENT). tmp + rename is atomic on POSIX. + await this._atomicWrite(data); } catch (error) { if (error.code === 'ELOCKED') { throw new Error('State file is locked by another process. Try again.'); @@ -134,8 +154,8 @@ class StateManager { // Apply update function const updatedData = await updateFn(currentData); - // Write updated data - await fs.writeFile(this.filePath, JSON.stringify(updatedData, null, 2), 'utf8'); + // Write updated data atomically (DC-170, same rationale as write()) + await this._atomicWrite(updatedData); return updatedData; } catch (error) { diff --git a/dashcaddy-api/src/utilities/middleware.js b/dashcaddy-api/src/utilities/middleware.js index 752e63f..1031bb8 100644 --- a/dashcaddy-api/src/utilities/middleware.js +++ b/dashcaddy-api/src/utilities/middleware.js @@ -138,7 +138,15 @@ module.exports = function configureMiddleware(app, { async function isIPInTailnet(clientTailscaleIP) { const status = await getTailscaleStatus(); - if (!status) return true; // no status = can't verify = allow + if (!status) { + // DC-170: 'no status' previously meant ALLOW — a tailscaled outage + // silently disabled the tailnet gate while the config claimed + // enforcement. With tailscaleConfig.failClosed set, deny instead; + // otherwise allow but say it loudly on stderr. + if (tailscaleConfig.failClosed) return false; + process.stderr.write('[tailscale] status unavailable — failing OPEN (set tailscale.failClosed=true to deny during outages)\n'); + return true; + } const knownIPs = new Set(); for (const ip of (status.Self?.TailscaleIPs || [])) knownIPs.add(ip); @@ -179,7 +187,13 @@ module.exports = function configureMiddleware(app, { }); } } catch (e) { - log.warn('tailscale', 'Tailnet verification failed, allowing request', { error: e.message }); + if (tailscaleConfig.failClosed) { + return errorResponse(res, 503, '[DC-121] Tailnet verification unavailable (fail-closed mode).', { + requiresTailscale: true, + clientIP + }); + } + log.warn('tailscale', 'Tailnet verification failed, allowing request (set tailscale.failClosed=true to deny during outages)', { error: e.message }); } } diff --git a/dashcaddy-api/src/websocket/dashboard-ws.js b/dashcaddy-api/src/websocket/dashboard-ws.js index 7b47ad8..174b16c 100644 --- a/dashcaddy-api/src/websocket/dashboard-ws.js +++ b/dashcaddy-api/src/websocket/dashboard-ws.js @@ -75,13 +75,12 @@ function createDashboardWS(server, deps = {}) { const authVerifier = typeof deps.authVerifier === 'function' ? deps.authVerifier : (req) => { - // Last-resort fallback: presence-only check on a non-empty - // session-cookie value. Used only when the caller didn't inject - // a real verifier (e.g. tests, unusual boot paths). Production - // wires the real one from app.locals.ctx.session.isValid. - const parsed = parseCookieHeader(req && req.headers && req.headers.cookie); - const raw = parsed.dashcaddy_session || parsed.sid; - return typeof raw === 'string' && raw.length > 0; + // DC-170 FAIL-CLOSED (audit 2026-10-27): the old fallback accepted ANY + // non-empty session cookie — the exact DC-061 footgun, resurfacing on + // boot paths that miss the verifier wiring. A missing verifier is a + // wiring bug: refuse everything and say so loudly. + process.stderr.write('[dashboard-ws] CRITICAL: no authVerifier injected — rejecting WS connection (fail closed). Wire ctx.session.isValid into deps.authVerifier.\n'); + return false; }; // Track connected clients and their subscriptions diff --git a/status/sw.js b/status/sw.js index 3896655..42e6a66 100644 --- a/status/sw.js +++ b/status/sw.js @@ -53,14 +53,18 @@ async function networkFirst(request, preloadResponsePromise) { try { const preloadResponse = preloadResponsePromise ? await preloadResponsePromise : null; if (preloadResponse) { - cache.put(request, preloadResponse.clone()).catch(() => {}); + if (preloadResponse.ok) cache.put(request, preloadResponse.clone()).catch(() => {}); return preloadResponse; } const response = await fetch(request); - cache.put(request, response.clone()).catch(() => {}); + // DC-170: only cache GOOD responses — a 5xx was being stored and later + // served as the app shell by both strategies. + if (response.ok) cache.put(request, response.clone()).catch(() => {}); return response; } catch (_) { - return caches.match(request) || caches.match('/index.html'); + // DC-170: `caches.match(x) || caches.match(y)` returned a Promise (always + // truthy) so the offline fallback never ran — await both sides. + return (await caches.match(request)) || (await caches.match('/index.html')); } } @@ -70,7 +74,7 @@ async function staleWhileRevalidate(request) { const networkPromise = fetch(request) .then((response) => { - cache.put(request, response.clone()).catch(() => {}); + if (response && response.ok) cache.put(request, response.clone()).catch(() => {}); return response; }) .catch(() => null); @@ -82,8 +86,13 @@ async function staleWhileRevalidate(request) { self.addEventListener('install', (event) => { self.skipWaiting(); event.waitUntil( + // DC-170: per-URL caching instead of one all-or-nothing addAll — a single + // renamed/missing asset previously rejected the whole install, leaving the + // OLD service worker (and its stale cache) in control indefinitely. caches.open(CACHE).then((cache) => - cache.addAll(PRECACHE.map((url) => new Request(url, { cache: 'reload' }))) + Promise.all(PRECACHE.map((url) => + cache.add(new Request(url, { cache: 'reload' })).catch(() => {}) + )) ) ); });