diff --git a/dashcaddy-api/__tests__/routes/errorresponse-arg-order.regression.test.js b/dashcaddy-api/__tests__/routes/errorresponse-arg-order.regression.test.js new file mode 100644 index 0000000..940349e --- /dev/null +++ b/dashcaddy-api/__tests__/routes/errorresponse-arg-order.regression.test.js @@ -0,0 +1,284 @@ +/** + * DC-063: errorResponse arg-order invariant regression suite. + * + * Three layers of correctness pinned by this test: + * + * (1) The validator at responses.js:76-98 catches wrong-order callers + * with a clear TypeError naming statusCode. Defense-in-depth: any + * future swap is caught at the smallest possible blast radius + * (one TypeError on the request thread) instead of an HTTP 500 HTML + * panic for the operator and client. + * + * (2) The static trees under dashcaddy-api/routes/ and + * dashcaddy-api/src/utilities/ follow ONE of two equivalent + * conventions consistently: + * + * Convention A — canonical import `errorResponse` from responses.js. + * Callsite shape: errorResponse(res, statusCode, message, extras?) + * statusCode must be an integer 100..599; message must be a string. + * + * Convention B — alias import `error: errorResponse` from responses.js, + * which binds the local `errorResponse` to the message-first + * helper `error(res, message, statusCode = 500)`. + * Callsite shape: errorResponse(res, message, statusCode) + * + * Mixing the alias-import with the canonical-shape callsite is the + * DC-063 bug class: at runtime, the alias function fires + * `res.status('event not found')` → TypeError → HTTP 500 HTML panic, + * silently masking the intended 4xx JSON response for the client. + * The validator at (1) does NOT help because the alias path skips it. + * + * (3) End-to-end smoke for one of each fixed-file: live HTTP hits the + * endpoint with the malformed input that triggers the fix-callsite + * branch, and asserts the wire response is the expected 4xx JSON + * (status + content-type + body) — never a 500 HTML panic. + * + * Origin (DC-062): shipped 2026-08-18 by Hermes loop. Found 4 callsites in + * routes/caddy-upstreams.js and added the validator. + * + * DC-063 (this file): extended the search across the routes tree with + * alias-import awareness. Found 18 instances of the alias-imported + + * canonical-shape callsite bug class in 2 files (security.js + 3 calls + * in services.js). Fixed by switching those imports to canonical and + * rewriting the remaining 4 alias-shape callsites in services.js to + * canonical-shape. Adding this regression test to prevent the same + * swap from being reintroduced in future route file edits. + */ + +const path = require('path'); +const express = require('express'); +const http = require('http'); +const fs = require('fs'); +const glob = require('glob'); + +const repoRoot = path.join(__dirname, '..', '..'); // dashcaddy-api/ +const { errorResponse, error: aliasError } = require( + path.join(repoRoot, 'src/utils/responses') +); + +// ─── (1) Type validator (defense-in-depth) ──────────────────────────────── +describe('DC-063: errorResponse type validator (defense-in-depth)', () => { + function makeRes() { + return { status: () => makeRes(), json: () => makeRes() }; + } + + test('canonical (res, statusCode, message) does not throw and JSON is well-formed', () => { + expect(() => errorResponse(makeRes(), 400, 'Invalid level')).not.toThrow(); + expect(() => errorResponse(makeRes(), 503, 'downstream unavailable', { code: 'DC-503' })) + .not.toThrow(); + }); + + test('swapped canonical-shape throws TypeError naming statusCode', () => { + expect(() => errorResponse(makeRes(), 'msg-not-status', 400)) + .toThrow(TypeError); + expect(() => errorResponse(makeRes(), 'msg-not-status', 400)) + .toThrow(/statusCode must be an integer HTTP status \(100\.\.599\)/); + }); + + test.each([ + [0, 'below range'], + [99, 'below range'], + [600, 'above range'], + [3.14, 'non-integer'], + [NaN, 'NaN'], + [Infinity, 'Infinity'], + ])('rejects numeric out-of-band statusCode %p (%s)', (bad) => { + expect(() => errorResponse(makeRes(), bad, 'msg')).toThrow(TypeError); + }); + + test('rejects non-string message', () => { + expect(() => errorResponse(makeRes(), 400, 42)).toThrow(TypeError); + expect(() => errorResponse(makeRes(), 400, null)).toThrow(TypeError); + expect(() => errorResponse(makeRes(), 400, { err: 'oops' })).toThrow(TypeError); + }); + + test('extras object merges into body and surfaces top-level code (DC-086)', () => { + const captured = {}; + const res = { + status(c) { captured.status = c; return res; }, + json(b) { captured.body = b; return res; }, + }; + errorResponse(res, 400, 'Invalid input', { code: 'DC-400', field: 'level' }); + expect(captured.status).toBe(400); + expect(captured.body).toEqual({ + success: false, + error: 'Invalid input', + field: 'level', + code: 'DC-400', + }); + }); + + test('alias error(res, message, statusCode) still works for backward-compat', () => { + expect(() => aliasError(makeRes(), 'msg', 400)).not.toThrow(); + }); +}); + +// ─── (2) Static tree: every callsite follows its file's imported convention ─ +describe('DC-063: routes/ + utilities/ arg-order matches each file\'s import', () => { + function isNumericLiteral(s) { + return /^\d+$/.test(s); + } + function isExpressionReturningNumber(s) { + return /^(err|error|response)\.status(Code)?\s*\|\|.*\d+/.test(s) || + /^response\.status$/.test(s); + } + function isStringy(s) { + s = s.trim(); + if (s.startsWith('"') || s.startsWith("'") || s.startsWith('`')) return true; + if (/^[a-zA-Z_][a-zA-Z_0-9]*\([^)]*\)$/.test(s)) return true; // safeErrorMessage(err) + if (/^[a-zA-Z_][a-zA-Z_0-9]*\.[a-zA-Z_][a-zA-Z_0-9.]*$/.test(s)) return true; // err.message + return false; + } + function isNumeric(s) { + return isNumericLiteral(s.trim()) || isExpressionReturningNumber(s.trim()); + } + // Match `errorResponse(res, ARG1, ARG2)` (allow extras after). + const pat = /errorResponse\(\s*res\s*,\s*([^,]+?)\s*,\s*([^,)\s]+)(?:\s*,|\s*\))/g; + + const ROUTES = glob.sync('routes/*.js', { cwd: repoRoot }); + const UTILS = glob.sync('src/utilities/*.js', { cwd: repoRoot }); + const ALL = [...ROUTES, ...UTILS]; + + function classifyFile(src) { + // Filter comments before classification (the comment can mention the alias). + const codeOnly = src.split('\n') + .filter((l) => !l.trim().startsWith('//') && !l.trim().startsWith('*') && !l.trim().startsWith('/*')) + .join('\n'); + const is_alias = /\berror:\s*errorResponse\b/.test(codeOnly); + return { is_alias }; + } + + test.each(ALL.map((rel) => [rel]))('%s has consistent callsite shape', (rel) => { + const abs = path.join(repoRoot, rel); + const src = fs.readFileSync(abs, 'utf8'); + const { is_alias } = classifyFile(src); + const bad = []; + for (const m of src.matchAll(pat)) { + const a1 = m[1].trim(); + const a2 = m[2].trim(); + const lineNo = src.slice(0, m.index).split('\n').length; + + if (is_alias) { + // Convention B: arg1 = message (string), arg2 = status (number) + if (isNumeric(a1) && isStringy(a2)) { + bad.push({ lineNo, a1, a2, reason: 'alias-import + canonical-shape (BUG: alias path skips validator)' }); + } + } else { + // Convention A: arg1 = status (number), arg2 = message (string) + if (isStringy(a1) && isNumeric(a2)) { + bad.push({ lineNo, a1, a2, reason: 'canonical-import + alias-shape (BUG: validator fires TypeError -> 500 HTML)' }); + } + } + } + if (bad.length) { + throw new Error( + `${rel}: ${bad.length} inconsistent callsite(s):\n` + + bad.map((b) => ` L${b.lineNo}: (${b.a1}, ${b.a2}) — ${b.reason}`).join('\n') + ); + } + }); +}); + +// ─── (3) End-to-end HTTP smoke — invalid input returns the expected JSON ─ +describe('DC-063: live HTTP smoke — security.js GET /events/:id returns 404 JSON (not 500)', () => { + let server, baseUrl; + + beforeAll(() => { + process.env.NODE_ENV = 'test'; + process.env.DASHCADDY_API_TOKEN = process.env.DASHCADDY_API_TOKEN || 'test-token'; + process.env.DASHCADDY_ENCRYPTION_KEY = process.env.DASHCADDY_ENCRYPTION_KEY || 'k'.repeat(64); + process.env.JWT_SECRET = process.env.JWT_SECRET || 'jwt-test-secret-32-chars-minimum-len'; + + const app = express(); + app.use(express.json()); + + // Auth shim — bypass host authentication middleware. + app.use((_req, _res, next) => next()); + + // Shim the security event store with a fake. + const fakeStore = { + get: () => null, + append: () => ({ id: 'fake', accepted: true }), + list: () => ({ events: [], total: 0 }), + query: () => ({ events: [], total: 0 }), + }; + const fakeRegistry = { + list: () => [], + register: () => ({ host: {}, api_key: 'x' }), + get: () => null, + update: () => null, + remove: () => true, + setEnabled: () => true, + authHostByApiKey: () => null, + authHostByBearer: () => null, + }; + + // Inject store + registry via a require-cache swap so security.js's + // getStore()/getRegistry() return our fakes. + require.cache[path.join(repoRoot, 'src/security/event-store')] = { + exports: { getStore: () => fakeStore }, + id: 'fake-event-store', filename: 'fake', loaded: true, + }; + require.cache[path.join(repoRoot, 'src/security/host-registry')] = { + exports: { getRegistry: () => fakeRegistry }, + id: 'fake-host-registry', filename: 'fake', loaded: true, + }; + // platform-paths is required by security.js — provide a minimal shim. + require.cache[path.join(repoRoot, 'platform-paths')] = { + exports: { configFile: () => '/tmp/x', dataFile: () => '/tmp/y' }, + id: 'fake-platform-paths', filename: 'fake', loaded: true, + }; + + const securityRoutes = require(path.join(repoRoot, 'routes/security')); + app.use((req, res, next) => { + res.success = (data) => res.json({ success: true, ...data }); + res.errorResponse = (status, msg, extras) => errorResponse(res, status, msg, extras); + res.ok = (data) => res.json({ success: true, ...data }); + next(); + }); + app.use('/api/security', securityRoutes({ + store: fakeStore, + registry: fakeRegistry, + log: { info: () => {}, warn: () => {}, error: () => {}, debug: () => {} }, + })); + + server = http.createServer(app).listen(0); + // .listen(0) synchronously assigns a port; no need to wait. + baseUrl = `http://127.0.0.1:${server.address().port}`; + }); + + afterAll((done) => { + if (server && server.listening) server.close(done); + else done(); + }); + + function get(p) { + return new Promise((resolve, reject) => { + http.get(`${baseUrl}${p}`, (resp) => { + let buf = ''; + resp.on('data', (c) => { buf += c; }); + resp.on('end', () => resolve({ + status: resp.statusCode, + body: buf, + contentType: resp.headers['content-type'] || '', + })); + }).on('error', reject); + }); + } + + test('GET /api/security/events/nonexistent — pre-fix crashed with 500 HTML, post-fix returns 404 JSON', async () => { + const r = await get('/api/security/events/nonexistent'); + expect(r.status).toBe(404); + expect(r.contentType).toMatch(/application\/json/); + expect(r.body).toMatch(/event not found/i); + expect(r.body).not.toMatch(/ { + const r = await get('/api/security/hosts/nonexistent'); + expect(r.status).toBe(404); + expect(r.contentType).toMatch(/application\/json/); + expect(r.body).toMatch(/host not found/i); + expect(r.body).not.toMatch(/ ({ parsePaginationParams: jest.fn(() => null), })); -jest.mock('../../src/utils/responses', () => ({ - success: jest.fn((res, data, statusCode = 200) => { - return res.status(statusCode).json({ success: true, ...data }); - }), - error: jest.fn((res, message, statusCode = 500, extra) => { - return res.status(statusCode).json({ success: false, error: message, ...extra }); - }), -})); +jest.mock('../../src/utils/responses', () => { + // DC-063: services.js now imports canonical `errorResponse` (statusCode-first), + // so this mock must expose both that AND the legacy `error` alias to keep the + // existing fixture working. The canonical validator is bypassed (tests use it + // as a structured passthrough); the alias preserves call-shape for any + // remaining legacy import. + const errorResponse = jest.fn((res, statusCode, message, extra) => + res.status(statusCode).json({ success: false, error: message, ...extra }) + ); + return { + success: jest.fn((res, data, statusCode = 200) => + res.status(statusCode).json({ success: true, ...data }) + ), + errorResponse, + error: errorResponse, // alias used by files that import `error: errorResponse` + }; +}); // errors module NOT mocked — used for real ValidationError/NotFoundError/ConflictError diff --git a/dashcaddy-api/routes/security.js b/dashcaddy-api/routes/security.js index 8e53457..b96863d 100644 --- a/dashcaddy-api/routes/security.js +++ b/dashcaddy-api/routes/security.js @@ -30,7 +30,11 @@ */ const express = require('express'); -const { ok, error: errorResponse } = require('../src/utils/responses'); +// DC-063: use the canonical `errorResponse(res, statusCode, message, extras)` +// shape — alias `error: errorResponse` used here previously was message-first +// which silently mis-called every callsite (15 endpoints surfaced as 500 HTML +// panics instead of the intended 4xx JSON). +const { ok, errorResponse } = require('../src/utils/responses'); const { getStore } = require('../src/security/event-store'); const { getRegistry } = require('../src/security/host-registry'); const platformPaths = require('../platform-paths'); diff --git a/dashcaddy-api/routes/services.js b/dashcaddy-api/routes/services.js index 36cd69b..540290d 100644 --- a/dashcaddy-api/routes/services.js +++ b/dashcaddy-api/routes/services.js @@ -10,7 +10,11 @@ const { exists } = require('../src/utilities/fs-helpers'); const { paginate, parsePaginationParams } = require('../src/utilities/pagination'); const { ValidationError, NotFoundError, ConflictError } = require('../src/utilities/errors'); const { resolveServiceUrl } = require('../src/utilities/url-resolver'); -const { success, error: errorResponse } = require('../src/utils/responses'); +// DC-063: use the canonical `errorResponse(res, statusCode, message, extras)` +// shape — alias `error: errorResponse` used here previously was message-first +// which silently mis-called 3 credential-store callsites (returned 500 HTML +// panics for invalid serviceId instead of the intended 400 JSON). +const { success, errorResponse } = require('../src/utils/responses'); const platformPaths = require('../platform-paths'); /** @@ -398,7 +402,8 @@ module.exports = function({ try { validateServiceConfig({ id, name }); } catch (validationErr) { - return errorResponse(res, validationErr.message, 400, { errors: validationErr.errors }); + // DC-063: canonical shape (res, statusCode, message, extras) per responses.js:76. + return errorResponse(res, 400, validationErr.message, { errors: validationErr.errors }); } await servicesStateManager.update(services => { @@ -423,7 +428,8 @@ module.exports = function({ } catch (error) { log.error('deploy', error, null, { note: 'Error adding service' }); if (error.message.includes('already exists')) { - errorResponse(res, safeErrorMessage(error), 409); + // DC-063: canonical shape per responses.js:76. + errorResponse(res, 409, safeErrorMessage(error)); } else { // Error handled by middleware } @@ -445,7 +451,8 @@ module.exports = function({ try { validateServiceConfig(service); } catch (validationErr) { - return errorResponse(res, `Invalid service "${service.id}": ${validationErr.message}`, 400, { errors: validationErr.errors }); + // DC-063: canonical shape per responses.js:76. + return errorResponse(res, 400, `Invalid service "${service.id}": ${validationErr.message}`, { errors: validationErr.errors }); } } @@ -475,7 +482,8 @@ module.exports = function({ }); if (!found) { - return errorResponse(res, `Service "${id}" not found`, 404); + // DC-063: canonical shape per responses.js:76. + return errorResponse(res, 404, `Service "${id}" not found`); } resyncHealthChecker?.().catch(() => {});