From 04aef0aa36afe4f0fd25980bde11d17bc252cfdb Mon Sep 17 00:00:00 2001 From: Thomas Joise Date: Mon, 20 Jul 2026 23:02:20 +0530 Subject: [PATCH] fix(api): validate ?days= and guard the onboarding-project routes against a crash (FIX B) GET /projects/stalled had no try/catch, and a caller-supplied ?days= flowed straight into `new Date(...).toISOString()`. Confirmed the exact crash: Number('1e300') passes Number.isFinite (so the old guard let it through), but the resulting cutoff math overflows the valid Date range and .toISOString() throws RangeError. Uncaught in an async Express handler this is an unhandled rejection, which kills the whole process on Node 22 -- any authenticated role, including plain staff, could take the console down with one GET. `days` is now validated (integer, 0-3650) with an explicit 4xx on anything hostile, falling back to the configured default only when the param is absent (matches the frontend, which never sends ?days= at all). Audited every other route this feature added to the same section: /projects/board and /projects/pending/:key had the identical no-try/catch shape and are now wrapped too. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/hq/src/api.ts | 39 +++++++++-- apps/hq/test/projects-stalled-route.test.ts | 75 +++++++++++++++++++++ 2 files changed, 107 insertions(+), 7 deletions(-) create mode 100644 apps/hq/test/projects-stalled-route.test.ts diff --git a/apps/hq/src/api.ts b/apps/hq/src/api.ts index ae5ed44..aff394f 100644 --- a/apps/hq/src/api.ts +++ b/apps/hq/src/api.ts @@ -782,18 +782,43 @@ export function apiRouter( }) // The cross-project board + drill-down (the "who's pending go-live" view). r.get('/projects/board', requireAuth, async (_req, res) => { - res.json({ ok: true, board: await milestoneBoard(db) }) + try { + res.json({ ok: true, board: await milestoneBoard(db) }) + } catch (err) { + res.status(400).json({ ok: false, error: err instanceof Error ? err.message : String(err) }) + } }) r.get('/projects/pending/:key', requireAuth, async (req, res) => { - res.json({ ok: true, projects: await projectsPendingMilestone(db, String(req.params['key'] ?? '')) }) + try { + res.json({ ok: true, projects: await projectsPendingMilestone(db, String(req.params['key'] ?? '')) }) + } catch (err) { + res.status(400).json({ ok: false, error: err instanceof Error ? err.message : String(err) }) + } }) // Parked onboarding: active projects whose next step hasn't moved in a while (dashboard/MIS tile). + // FIX B (pre-ship audit): a caller-supplied `days` used to flow straight into `new Date(...)` + // math with no bound and no try/catch — an out-of-range/NaN value threw a RangeError that, + // as an unhandled rejection, crashed the whole process. `days` is now validated (finite + // integer, sane bounds) and the handler is wrapped like its siblings. r.get('/projects/stalled', requireAuth, async (req, res) => { - const q = req.query['days'] - const days = typeof q === 'string' && q !== '' && Number.isFinite(Number(q)) - ? Number(q) : await getNumberSetting(db, 'project.stall_days', 30) - const today = new Date().toISOString().slice(0, 10) - res.json({ ok: true, projects: await stalledProjects(db, days, today) }) + try { + const q = req.query['days'] + let days: number + if (typeof q === 'string' && q !== '') { + const parsed = Number(q) + if (!Number.isInteger(parsed) || parsed < 0 || parsed > 3650) { + res.status(400).json({ ok: false, error: 'days must be an integer between 0 and 3650' }) + return + } + days = parsed + } else { + days = await getNumberSetting(db, 'project.stall_days', 30) + } + const today = new Date().toISOString().slice(0, 10) + res.json({ ok: true, projects: await stalledProjects(db, days, today) }) + } catch (err) { + res.status(400).json({ ok: false, error: err instanceof Error ? err.message : String(err) }) + } }) // The shared onboarding "front" — same lead-in steps for every module's checklist (owner editor). diff --git a/apps/hq/test/projects-stalled-route.test.ts b/apps/hq/test/projects-stalled-route.test.ts new file mode 100644 index 0000000..c776045 --- /dev/null +++ b/apps/hq/test/projects-stalled-route.test.ts @@ -0,0 +1,75 @@ +// apps/hq/test/projects-stalled-route.test.ts — pre-ship audit FIX B: GET /projects/stalled +// had no try/catch, and a caller-supplied `?days=` flowed straight into +// `new Date(...).toISOString()`, which throws a RangeError on an out-of-range/NaN value. +// On Express 4 + Node 22 an unhandled rejection from an async route handler crashes the +// whole process — ANY authenticated role (including plain staff) could take the console +// down with one GET. `days` is now validated and the handler wrapped like its siblings. +import express from 'express' +import { describe, it, expect, afterAll } from 'vitest' +import { openDb } from '../src/db' +import { seedIfEmpty } from '../src/seed' +import { apiRouter } from '../src/api' +import { createStaff } from '../src/auth' + +describe('GET /projects/stalled — hostile ?days= must not crash the server', () => { + const servers: { close: () => void }[] = [] + afterAll(() => { for (const s of servers) s.close() }) + + async function httpWorld(): Promise<{ base: string; staffH: Record }> { + const db = openDb(':memory:') + await seedIfEmpty(db) + await createStaff(db, { email: 'staff-stalled@t.in', displayName: 'Staff', role: 'staff', password: 'staff-pass-1' }) + const app = express(); app.use(express.json()); app.locals['db'] = db + app.use('/api', apiRouter(db)) + const server = app.listen(0); servers.push(server) + const base = `http://localhost:${(server.address() as { port: number }).port}/api` + const token = ((await (await fetch(`${base}/auth/login`, { + method: 'POST', headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ email: 'staff-stalled@t.in', password: 'staff-pass-1' }), + })).json()) as { token: string }).token + return { base, staffH: { authorization: `Bearer ${token}` } } + } + + it('a plain request (no ?days=) works — falls back to the configured default', async () => { + const { base, staffH } = await httpWorld() + const res = await fetch(`${base}/projects/stalled`, { headers: staffH }) + expect(res.status).toBe(200) + const body = await res.json() as { ok: boolean; projects: unknown[] } + expect(body.ok).toBe(true) + expect(Array.isArray(body.projects)).toBe(true) + }) + + it('a sane ?days= still works normally', async () => { + const { base, staffH } = await httpWorld() + const res = await fetch(`${base}/projects/stalled?days=45`, { headers: staffH }) + expect(res.status).toBe(200) + expect((await res.json() as { ok: boolean }).ok).toBe(true) + }) + + const hostileValues = [ + '1e300', // finite in JS but overflows Date math -> used to throw RangeError + '-99999999999999', // huge negative -> overflows the other direction + '3.5', // not an integer + 'abc', // NaN + 'NaN', + 'Infinity', + '-Infinity', + '99999999999999999999', // way outside the sane 0-3650 bound + '-1', // out of bound (negative) + ] + + for (const hostile of hostileValues) { + it(`?days=${hostile} is rejected with a 4xx, never throws / crashes the server`, async () => { + const { base, staffH } = await httpWorld() + const res = await fetch(`${base}/projects/stalled?days=${encodeURIComponent(hostile)}`, { headers: staffH }) + expect(res.status).toBeGreaterThanOrEqual(400) + expect(res.status).toBeLessThan(500) + const body = await res.json() as { ok: boolean } + expect(body.ok).toBe(false) + // The server process must still be alive for the NEXT request — the original bug's + // unhandled RangeError rejection killed the whole process. + const health = await fetch(`${base}/health`) + expect(health.status).toBe(200) + }) + } +})