[grade=A urn:ump:qszwspay7vhx7wcpqekkkidni3ehhgidvze5qk2y22fvg2wqz3ja] fix(DC-170): reliability batch — hangs, atomicity, fail-closed, SW (audit batch 6)

8 reliability fixes across 8 files (AUDIT-2026-10-27.md findings
9/11/12/13/14/16/20 + calendar credential item):

- dns.js check-update/update HANGS: bare returns + empty else/catch
  branches left requests hanging until client timeout; every path now
  answers (502 with message); upstream JSON.parse wrapped (non-JSON ->
  502)
- calendar.js ICS fetch timeout actually applies: fetchT ignores
  opts.timeout (undici) — was running at 5s default, not the intended
  15s; now timeoutMs third arg
- calendar.js PUT /sources: updating pass no longer nulls the stored
  token (and vice versa) — existing credential record loaded, only the
  provided field overwritten
- state-manager.js ATOMIC WRITES: _atomicWrite = tmp file + fs.rename
  (POSIX-atomic); write()/update() no longer truncate-then-write the
  live file (crash mid-write corrupted services.json/config.json with
  no recovery path). Test mock extended to the tmp+rename contract
- dashboard-ws.js auth fallback FAIL-CLOSED: missing authVerifier used
  to accept any non-empty session cookie (DC-061 footgun on unwired
  boot paths); now rejects + loud CRITICAL log
- middleware.js tailscale gate: new tailscale.failClosed config (default
  off = unchanged); when set, outages/errors deny (503) instead of
  silently disabling the gate; fail-open path now logs the exact key
- sw.js: offline fallback actually works ((await match) || (await
  match) — old Promise||Promise was always truthy); only 2xx cached
  (5xx pages were stored and served as the app shell); install is
  per-URL tolerant (one bad asset kept the stale SW alive forever)
- config/settings.js GET /config (public): corrupt config.json degrades
  to {setupComplete:false, corrupt:true} instead of 500 on every load

SCOPE CORRECTION: audit finding #10 (7 reversed-arg errorResponse
sites) was a FALSE POSITIVE — those files alias message-first error();
see lesson #14. Full suite 173/173 suites, 3740/3740 tests.
This commit is contained in:
Hermes committed 2026-10-29 01:46:13 -07:00
1 parent f4f2c26de3
commit 6b7ca4b3e9
8 files changed
+115 -31

No files matched your search

+10 -2
View File
@@ -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();
});
+17 -2
View File
@@ -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.
+9 -1
View File
@@ -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'));
+19 -8
View File
@@ -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'));
+24 -4
View File
@@ -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) {
+16 -2
View File
@@ -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 });
}
}
+6 -7
View File
@@ -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