Routes/caddy-upstreams.js had 4 callsites with argument-order swapped: errorResponse(res, 'message', 503) instead of errorResponse(res, 503, 'message'). The canonical signature from src/utils/responses.js:66 takes statusCode FIRST; the swapped call passed a STRING where Express expected a status code. res.status('Caddy upstream watcher not initialized') throws RangeError [ERR_HTTP_INVALID_STATUS_CODE], Express's error middleware catches it, and the response is 500 with an HTML stack trace instead of the intended 503 JSON. Four `!caddyUpstreamWatcher` defensive guards had this exact pattern; all fixed.
Defense-in-depth (responses.js): errorResponse() now validates that statusCode is an integer in 100..599 and that message is a string BEFORE calling res.status(). Future arg-order mistakes fail fast with a clear TypeError naming the wrong arg and the message — instead of writing a 500 HTML panic to the wire. Legacy error(res, message, statusCode) helper (used by ~7 files that import as 'error: errorResponse' alias) is intentionally untouched.
Tests (__tests__/utils-responses-dc-062.test.js, NEW, 21 tests pass):
- correct (res, 503, msg) order: 503 JSON
- swapped (res, msg, statusCode) order: TypeError (was: silent 500 HTML panic)
- 10 invalid-statusCode cases: NaN, Infinity, '503', null, undefined, underflow, overflow, float, object, array — all rejected
- non-string message rejected
- DC-086 extras.code propagation preserved
- legacy error() helper regression: still works
- pre-fix Express server proves the bug class (500 HTML when statusCode is a string)
- all 4 caddy-upstreams routes with null watcher now return 503 JSON
- static source scan: 0 swapped patterns, 4 canonical (statusCode, 'message') occurrences
Full suite: 92 suites / 2039 tests / all green pre and post fix.
[glm-grade=A] from deleg_45e44614 (3 tool calls, 82s, MiniMax-M3 stand-in per Sami's 2026-08-17 authorization)
332 lines
13 KiB
JavaScript
332 lines
13 KiB
JavaScript
/**
|
|
* DC-062: errorResponse arg-order regression test + caddy-upstreams JSON
|
|
* response guarantees.
|
|
*
|
|
* Background: errorResponse(res, statusCode, message, extras) is the canonical
|
|
* shape from src/utils/responses.js. Routes that import the bare
|
|
* `errorResponse` (not the `error: errorResponse` alias) MUST call it
|
|
* statusCode-first. The classic bug is `errorResponse(res, 'message', 503)`
|
|
* — Express rejects the string with RangeError [ERR_HTTP_INVALID_STATUS_CODE]
|
|
* and writes a 500 with an HTML stack trace instead of the intended 503 JSON.
|
|
*
|
|
* DC-049 (caddy-upstream-watcher, shipped 2026-08-18) had 4 instances of this
|
|
* exact pattern in its route file, in the `!caddyUpstreamWatcher` defensive
|
|
* branch. The branch is currently unreachable in prod (the watcher is always
|
|
* wired in app.js:818-822) but the latent bug is a 1) crash-handler failure
|
|
* mode if the watcher module ever errored at load time, 2) wrong response
|
|
* shape (HTML instead of JSON), and 3) HTTP 500 instead of the intended 503.
|
|
*
|
|
* Two layers of fix:
|
|
* 1. routes/caddy-upstreams.js — swap the 4 callsites to (res, 503, msg).
|
|
* 2. src/utils/responses.js — add a defensive arg validator on
|
|
* errorResponse() so any future (res, <not-a-valid-status>, ...)
|
|
* call FAILS FAST with a clear TypeError instead of writing a 500 HTML
|
|
* panic to the client. The older `error()` helper (message-first,
|
|
* imported as `error: errorResponse`) intentionally preserves its
|
|
* existing API and is untouched.
|
|
*
|
|
* This test exercises both fixes.
|
|
*/
|
|
|
|
const express = require('express');
|
|
const http = require('http');
|
|
const path = require('path');
|
|
|
|
// Use the repo's deps so the test fails under exactly the same module
|
|
// resolution as production code (otherwise symlink/path differences can
|
|
// mask validator-install gaps).
|
|
// __dirname = /opt/dashcaddy/dashcaddy-api/__tests__
|
|
// __dirname/../src/utils/responses = the file under test
|
|
const repoRoot = path.join(__dirname, '..');
|
|
|
|
const { errorResponse, error: legacyError } = require(path.join(repoRoot, 'src/utils/responses'));
|
|
|
|
function get(port, urlPath) {
|
|
return new Promise((resolve, reject) => {
|
|
const req = http.get(`http://localhost:${port}${urlPath}`, (resp) => {
|
|
let body = '';
|
|
resp.on('data', (c) => { body += c; });
|
|
resp.on('end', () => resolve({ status: resp.statusCode, headers: resp.headers, body }));
|
|
});
|
|
req.on('error', reject);
|
|
});
|
|
}
|
|
|
|
describe('errorResponse canonical arg-order + type guard (DC-062)', () => {
|
|
test('correct order — (res, 503, msg) returns 503 JSON', () => {
|
|
const mockRes = {
|
|
status(code) { mockRes._code = code; return this; },
|
|
json(body) { mockRes._body = body; return this; },
|
|
};
|
|
errorResponse(mockRes, 503, 'Caddy upstream watcher not initialized');
|
|
expect(mockRes._code).toBe(503);
|
|
expect(mockRes._body).toEqual({ success: false, error: 'Caddy upstream watcher not initialized' });
|
|
});
|
|
|
|
test('swapped order — (res, msg, statusCode) throws TypeError instead of writing a 500 HTML panic', () => {
|
|
// Before DC-062: errorResponse would call res.status('string-msg'),
|
|
// Express throws RangeError, error middleware catches it, writes 500 HTML.
|
|
// After DC-062: errorResponse itself rejects the call with a clear
|
|
// TypeError, naming the wrong arg.
|
|
const mockRes = {
|
|
status: () => mockRes,
|
|
json: () => mockRes,
|
|
};
|
|
expect(() => errorResponse(mockRes, 'Caddy upstream watcher not initialized', 503))
|
|
.toThrow(TypeError);
|
|
expect(() => errorResponse(mockRes, 'Caddy upstream watcher not initialized', 503))
|
|
.toThrow(/statusCode must be an integer HTTP status/);
|
|
});
|
|
|
|
test.each([
|
|
['NaN', NaN],
|
|
['Infinity', Infinity],
|
|
['string "503"', '503'],
|
|
['null', null],
|
|
['undefined', undefined],
|
|
['underflow 99', 99],
|
|
['overflow 600', 600],
|
|
['float 503.5', 503.5],
|
|
['object', { code: 503 }],
|
|
['array', [503]],
|
|
])('rejects invalid statusCode %s', (_name, badStatus) => {
|
|
const mockRes = {
|
|
status: () => mockRes,
|
|
json: () => mockRes,
|
|
};
|
|
expect(() => errorResponse(mockRes, badStatus, 'msg')).toThrow(TypeError);
|
|
});
|
|
|
|
test('rejects non-string message', () => {
|
|
const mockRes = {
|
|
status: () => mockRes,
|
|
json: () => mockRes,
|
|
};
|
|
expect(() => errorResponse(mockRes, 503, 123)).toThrow(TypeError);
|
|
expect(() => errorResponse(mockRes, 503, null)).toThrow(TypeError);
|
|
expect(() => errorResponse(mockRes, 503, undefined)).toThrow(TypeError);
|
|
expect(() => errorResponse(mockRes, 503, { msg: 'x' })).toThrow(TypeError);
|
|
});
|
|
|
|
test('preserves correct callers (DC-086 extras.code propagation still works)', () => {
|
|
const mockRes = {
|
|
status: () => mockRes,
|
|
json: (b) => { mockRes._lastBody = b; return mockRes; },
|
|
};
|
|
errorResponse(mockRes, 409, 'Conflict', { code: 'DC-CONF-1', extra: 'detail' });
|
|
expect(mockRes._lastBody).toEqual({
|
|
success: false,
|
|
error: 'Conflict',
|
|
code: 'DC-CONF-1',
|
|
extra: 'detail',
|
|
});
|
|
});
|
|
|
|
test('legacy `error()` helper (message, status) is UNCHANGED — still works', () => {
|
|
// Regression guard for alias-style importers (dns.js, services.js,
|
|
// ssl-monitor.js, license.js, dependencies.js, errorlogs.js, etc.).
|
|
// The legacy helper takes (res, message, statusCode) order. Make sure
|
|
// the validator we added to `errorResponse` doesn't bleed into
|
|
// `error()`.
|
|
const mockRes = {
|
|
status(code) { mockRes._code = code; return this; },
|
|
json(body) { mockRes._body = body; return this; },
|
|
};
|
|
legacyError(mockRes, 'service unavailable', 503);
|
|
expect(mockRes._code).toBe(503);
|
|
expect(mockRes._body).toEqual({ success: false, error: 'service unavailable' });
|
|
});
|
|
|
|
test('regression: an Express response with res.status(string) emits HTML 500 — proves the bug pre-fix', async () => {
|
|
// This is the failure mode DC-062 prevents. We still need this to
|
|
// be true to prove the guard's value: if a call site ever slipped past
|
|
// the validator (e.g. by sending a non-number disguised as code 0),
|
|
// the server still doesn't return the intended status as JSON.
|
|
const server = await new Promise((resolve) => {
|
|
const app = express();
|
|
app.get('/probe', (req, res) => {
|
|
try {
|
|
res.status('not a status').json({ ok: false });
|
|
} catch (_) {
|
|
res.end();
|
|
}
|
|
});
|
|
const s = app.listen(0, () => resolve({
|
|
port: s.address().port,
|
|
close: () => new Promise((r) => s.close(r)),
|
|
}));
|
|
});
|
|
try {
|
|
const resp = await get(server.port, '/probe');
|
|
expect(resp.status).toBe(500);
|
|
// Express renders an HTML error page (not JSON) — this is the bug
|
|
// class DC-062 prevents at the helper layer.
|
|
expect(resp.headers['content-type'] || '').toMatch(/text\/html/);
|
|
} finally {
|
|
await server.close();
|
|
}
|
|
});
|
|
});
|
|
|
|
// Mount the real route module and inject a null watcher — proves the
|
|
// the four `!caddyUpstreamWatcher` paths now respond with the intended
|
|
// 503 JSON shape, not a 500 HTML panic.
|
|
describe('caddy-upstreams JSON response shape (route file literal fix)', () => {
|
|
// The real route module exports a factory `function({ asyncHandler, caddyUpstreamWatcher, healthChecker })`.
|
|
// We need to provide an asyncHandler shim since the route file uses it.
|
|
function asyncHandlerShim(fn) { return fn; }
|
|
// The factory also depends on the asyncHandler resolving rejected
|
|
// promises to errors. Define a simple one that just calls next(err).
|
|
function asyncHandler(fn) {
|
|
return (req, res, next) => {
|
|
Promise.resolve(fn(req, res, next)).catch(next);
|
|
};
|
|
}
|
|
|
|
function mountRouter(router) {
|
|
return new Promise((resolve) => {
|
|
const app = express();
|
|
app.use('/api/v1', router);
|
|
const server = app.listen(0, () => resolve({
|
|
port: server.address().port,
|
|
close: () => new Promise((r) => server.close(r)),
|
|
}));
|
|
});
|
|
}
|
|
|
|
function loadRoute(deps) {
|
|
return require(path.join(repoRoot, 'routes/caddy-upstreams'))(deps);
|
|
}
|
|
|
|
test('GET /caddy/upstreams with null watcher — 503 JSON (regression for swap bug)', async () => {
|
|
const router = loadRoute({
|
|
asyncHandler,
|
|
caddyUpstreamWatcher: null,
|
|
healthChecker: null,
|
|
});
|
|
const server = await mountRouter(router);
|
|
try {
|
|
const resp = await get(server.port, '/api/v1/caddy/upstreams');
|
|
expect(resp.status).toBe(503);
|
|
expect(resp.body).toContain('"success":false');
|
|
expect(resp.body).toContain('Caddy upstream watcher not initialized');
|
|
expect(resp.headers['content-type'] || '').toMatch(/application\/json/);
|
|
} finally {
|
|
await server.close();
|
|
}
|
|
});
|
|
|
|
test('POST /caddy/upstreams/:host/mute with null watcher — 503 JSON', async () => {
|
|
const router = loadRoute({
|
|
asyncHandler,
|
|
caddyUpstreamWatcher: null,
|
|
healthChecker: null,
|
|
});
|
|
const server = await mountRouter(router);
|
|
try {
|
|
const req = http.request({
|
|
hostname: 'localhost',
|
|
port: server.port,
|
|
method: 'POST',
|
|
path: '/api/v1/caddy/upstreams/100.74.102.61:8080/mute',
|
|
}, (res) => {
|
|
let body = '';
|
|
res.on('data', (c) => { body += c; });
|
|
res.on('end', () => {
|
|
expect(res.statusCode).toBe(503);
|
|
expect(body).toContain('"success":false');
|
|
expect(body).toContain('Caddy upstream watcher not initialized');
|
|
expect(res.headers['content-type'] || '').toMatch(/application\/json/);
|
|
server.close();
|
|
});
|
|
});
|
|
req.on('error', (e) => { throw e; });
|
|
req.end();
|
|
} finally {
|
|
// server.close() will run via res.on('end') — defensively guard too.
|
|
// (Don't double-close if test already returned.)
|
|
}
|
|
});
|
|
|
|
test('POST /caddy/upstreams/mute (bare) with null watcher — 503 JSON', async () => {
|
|
const router = loadRoute({
|
|
asyncHandler,
|
|
caddyUpstreamWatcher: null,
|
|
healthChecker: null,
|
|
});
|
|
const server = await mountRouter(router);
|
|
try {
|
|
const resp = await new Promise((resolve, reject) => {
|
|
const req = http.request({
|
|
hostname: 'localhost',
|
|
port: server.port,
|
|
method: 'POST',
|
|
path: '/api/v1/caddy/upstreams/mute',
|
|
headers: { 'Content-Type': 'application/json' },
|
|
}, (res) => {
|
|
let body = '';
|
|
res.on('data', (c) => { body += c; });
|
|
res.on('end', () => resolve({ status: res.statusCode, headers: res.headers, body }));
|
|
});
|
|
req.on('error', reject);
|
|
req.end('{"host":"x","muted":true}');
|
|
});
|
|
expect(resp.status).toBe(503);
|
|
expect(resp.body).toContain('Caddy upstream watcher not initialized');
|
|
expect(resp.headers['content-type'] || '').toMatch(/application\/json/);
|
|
} finally {
|
|
await server.close();
|
|
}
|
|
});
|
|
|
|
test('POST /caddy/upstreams/:host/unmute with null watcher — 503 JSON', async () => {
|
|
const router = loadRoute({
|
|
asyncHandler,
|
|
caddyUpstreamWatcher: null,
|
|
healthChecker: null,
|
|
});
|
|
const server = await mountRouter(router);
|
|
try {
|
|
const resp = await new Promise((resolve, reject) => {
|
|
const req = http.request({
|
|
hostname: 'localhost',
|
|
port: server.port,
|
|
method: 'POST',
|
|
path: '/api/v1/caddy/upstreams/100.74.102.61:8080/unmute',
|
|
}, (res) => {
|
|
let body = '';
|
|
res.on('data', (c) => { body += c; });
|
|
res.on('end', () => resolve({ status: res.statusCode, headers: res.headers, body }));
|
|
});
|
|
req.on('error', reject);
|
|
req.end();
|
|
});
|
|
expect(resp.status).toBe(503);
|
|
expect(resp.body).toContain('Caddy upstream watcher not initialized');
|
|
expect(resp.headers['content-type'] || '').toMatch(/application\/json/);
|
|
} finally {
|
|
await server.close();
|
|
}
|
|
});
|
|
|
|
test('route file source: no swapped-order patterns remain', () => {
|
|
// Static scan of the post-fix route file: confirms the 4 swapped calls
|
|
// are gone. If a future refactor re-introduces the pattern, this scan
|
|
// catches it at test-time (before it ever lands in prod).
|
|
const fs = require('fs');
|
|
const src = fs.readFileSync(
|
|
path.join(repoRoot, 'routes/caddy-upstreams.js'),
|
|
'utf8'
|
|
);
|
|
// Match `errorResponse(res, <quote-or-backtick>, <int>)` — the
|
|
// swapped-order shape (string literal in the 2nd arg position).
|
|
const swappedRe = /errorResponse\(res,\s*['"`]/;
|
|
expect(src).not.toMatch(swappedRe);
|
|
// And confirm the corrected shape appears at least four times
|
|
// (the four `!caddyUpstreamWatcher` guards).
|
|
const canonicalRe = /errorResponse\(res,\s*503,\s*['"]Caddy upstream watcher not initialized['"]/g;
|
|
const matches = src.match(canonicalRe) || [];
|
|
expect(matches.length).toBe(4);
|
|
});
|
|
});
|