From c01a011d4764eee99d04f2a1e3227c53f51c7c64 Mon Sep 17 00:00:00 2001 From: DashCaddy Polish Loop Date: Tue, 18 Aug 2026 09:03:58 -0700 Subject: [PATCH] [glm-grade=A] fix(caddy-upstreams): swap errorResponse arg order to statusCode-first; add type validator (DC-062) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../__tests__/utils-responses-dc-062.test.js | 331 ++++++++++++++++++ dashcaddy-api/routes/caddy-upstreams.js | 15 +- dashcaddy-api/src/utils/responses.js | 26 ++ 3 files changed, 368 insertions(+), 4 deletions(-) create mode 100644 dashcaddy-api/__tests__/utils-responses-dc-062.test.js diff --git a/dashcaddy-api/__tests__/utils-responses-dc-062.test.js b/dashcaddy-api/__tests__/utils-responses-dc-062.test.js new file mode 100644 index 0000000..3166c2e --- /dev/null +++ b/dashcaddy-api/__tests__/utils-responses-dc-062.test.js @@ -0,0 +1,331 @@ +/** + * 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, , ...) + * 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, , )` — 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); + }); +}); diff --git a/dashcaddy-api/routes/caddy-upstreams.js b/dashcaddy-api/routes/caddy-upstreams.js index 851d8cc..79f28d0 100644 --- a/dashcaddy-api/routes/caddy-upstreams.js +++ b/dashcaddy-api/routes/caddy-upstreams.js @@ -21,7 +21,14 @@ module.exports = function({ asyncHandler, caddyUpstreamWatcher, healthChecker }) router.get('/caddy/upstreams', asyncHandler(async (req, res) => { if (!caddyUpstreamWatcher) { - return errorResponse(res, 'Caddy upstream watcher not initialized', 503); + // DC-062: errorResponse(res, statusCode, message) — statusCode-first per + // src/utils/responses.js:66. The prior (res, message, statusCode) call + // order passed a STRING as the status code, which made + // res.status('Caddy upstream watcher not initialized') throw + // RangeError [ERR_HTTP_INVALID_STATUS_CODE] (Express turning it into a + // 500 with an HTML stack trace). All four `!caddyUpstreamWatcher` + // guards had the same latent bug — fixed to canonical order. + return errorResponse(res, 503, 'Caddy upstream watcher not initialized'); } success(res, caddyUpstreamWatcher.snapshot()); }, 'caddy-upstreams-list')); @@ -54,7 +61,7 @@ module.exports = function({ asyncHandler, caddyUpstreamWatcher, healthChecker }) // ergonomic depending on caller. const handleMute = asyncHandler(async (req, res) => { if (!caddyUpstreamWatcher) { - return errorResponse(res, 'Caddy upstream watcher not initialized', 503); + return errorResponse(res, 503, 'Caddy upstream watcher not initialized'); } const host = req.params.host || req.body?.host; if (!host || typeof host !== 'string' || !/^[a-z0-9._:-]+$/i.test(host)) { @@ -76,7 +83,7 @@ module.exports = function({ asyncHandler, caddyUpstreamWatcher, healthChecker }) // absent or unparseable; require muted === false explicitly to unmute. router.post('/caddy/upstreams/mute', asyncHandler(async (req, res) => { if (!caddyUpstreamWatcher) { - return errorResponse(res, 'Caddy upstream watcher not initialized', 503); + return errorResponse(res, 503, 'Caddy upstream watcher not initialized'); } const { host, muted } = req.body || {}; if (!host || typeof host !== 'string' || !/^[a-z0-9._:-]+$/i.test(host)) { @@ -95,7 +102,7 @@ module.exports = function({ asyncHandler, caddyUpstreamWatcher, healthChecker }) router.post('/caddy/upstreams/:host/mute', handleMute); router.post('/caddy/upstreams/:host/unmute', asyncHandler(async (req, res) => { if (!caddyUpstreamWatcher) { - return errorResponse(res, 'Caddy upstream watcher not initialized', 503); + return errorResponse(res, 503, 'Caddy upstream watcher not initialized'); } const host = req.params.host; if (!host || !/^[a-z0-9._:-]+$/i.test(host)) { diff --git a/dashcaddy-api/src/utils/responses.js b/dashcaddy-api/src/utils/responses.js index 0cf97c5..f0c9cd4 100644 --- a/dashcaddy-api/src/utils/responses.js +++ b/dashcaddy-api/src/utils/responses.js @@ -62,8 +62,34 @@ function noContent(res) { * * DC-086: If extras.code is set, it's treated as a machine-readable error code * (e.g. 'DC-CONT-002'). If message looks like a DC code, it's auto-extracted. + * + * DC-062: Validate that `statusCode` is a valid HTTP status (integer in + * 100..599) BEFORE calling res.status(). Without this guard, a caller who + * passes (res, message, statusCode) instead of (res, statusCode, message) + * ends up with res.status(), which throws + * RangeError [ERR_HTTP_INVALID_STATUS_CODE] — Express catches that and + * writes a 500 with an HTML stack trace to the client, which is the worst + * possible failure mode (looks like a server crash, breaks CSRF and + * content-type expectations, leaks the stack). Failing fast with a clear + * TypeError names the call site early in the request lifecycle. */ function errorResponse(res, statusCode, message, extras = {}) { + if ( + typeof statusCode !== 'number' + || !Number.isFinite(statusCode) + || !Number.isInteger(statusCode) + || statusCode < 100 + || statusCode > 599 + ) { + throw new TypeError( + `errorResponse(res, statusCode, message, extras): statusCode must be an integer HTTP status (100..599); received ${JSON.stringify(statusCode)} (message=${JSON.stringify(message)})` + ); + } + if (typeof message !== 'string') { + throw new TypeError( + `errorResponse(res, statusCode, message, extras): message must be a string; received ${typeof message} ${JSON.stringify(message)}` + ); + } const body = { success: false, error: message, ...extras }; // DC-086: surface machine-readable code at top level for client handling if (extras.code) {