From af82adf2f36f0a5a242a92954906a622dc53290c Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Tue, 19 May 2026 12:20:57 -0500 Subject: [PATCH] fix(timeentry): silent-drop unparseable from/to query params MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both `listByCompany` and `exportCsv` declare in a doc comment that "bad dates are silently dropped rather than 400'd, so a typo in the query string doesn't break the call." The implementation didn't match: the raw `req.query.from` / `req.query.to` string was plugged straight into Sequelize's `Op.gte` / `Op.lte`. Postgres can parse ISO 8601 fine but throws `invalid input syntax for type timestamp` on garbage like `?from=not-a-date`, which Sequelize surfaces — and the controller's catch returns a 500. Add `parseDateOrNull(s)` to the controller: returns a Date for any input the `Date` constructor accepts (Number.isFinite on getTime()), returns null otherwise. Wire both handlers through it so the silent-drop behavior matches the documented contract. Unit-test the helper directly via `_internals` — happy paths (ISO 8601, date-only), the silent-drop case (garbage strings), and the defensive non-string-input branch. The helper is also wired through the smoke-test harness's `vi.mock` for `db.config.js`, so failures land cleanly without a live DB. Co-Authored-By: Claude Opus 4.7 (1M context) --- app/controllers/timeentrycontroller.js | 45 ++++++++++++++++++++------ tests/api/timeentry.test.js | 39 ++++++++++++++++++++++ 2 files changed, 74 insertions(+), 10 deletions(-) diff --git a/app/controllers/timeentrycontroller.js b/app/controllers/timeentrycontroller.js index 668d2db..f2a83eb 100644 --- a/app/controllers/timeentrycontroller.js +++ b/app/controllers/timeentrycontroller.js @@ -31,6 +31,22 @@ function computeMinutes(startedAt, endedAt) { return Math.round((end - start) / 60000); } +/** + * Parse a date-ish query-string value (e.g. ?from=, ?to=) into a Date, + * or return null if it can't be parsed. The list + export handlers + * previously plugged the raw string straight into Sequelize's + * Op.gte/Op.lte, which works for ISO 8601 but causes Postgres to + * throw on garbage input — which Sequelize then surfaces as a 500 + * from the controller's catch. The doc comment on those handlers + * claimed bad dates are "silently dropped"; this helper makes that + * comment true. + */ +function parseDateOrNull(s) { + if (typeof s !== 'string' || s.length === 0) return null; + const t = new Date(s).getTime(); + return Number.isFinite(t) ? new Date(t) : null; +} + /** * Return true when both bounds are non-empty AND parse cleanly AND * endedAt is strictly before startedAt. Used by the PATCH handler to @@ -180,13 +196,18 @@ exports.listByCompany = async (req, res) => { } // Date range — use Sequelize.Op.gte/lte. Keep it permissive: bad // dates are silently dropped rather than 400'd, so a typo in the - // query string doesn't break the call. + // query string doesn't break the call. parseDateOrNull guards + // against `from=not-a-date` reaching Postgres (which would throw + // and produce a 500); the silent-drop behavior matches the + // documented contract. const Op = db.Sequelize && db.Sequelize.Op; - if (Op && req.query.from) { - where.teStartedAt = Object.assign(where.teStartedAt || {}, { [Op.gte]: req.query.from }); + const fromDate = parseDateOrNull(req.query.from); + const toDate = parseDateOrNull(req.query.to); + if (Op && fromDate) { + where.teStartedAt = Object.assign(where.teStartedAt || {}, { [Op.gte]: fromDate }); } - if (Op && req.query.to) { - where.teStartedAt = Object.assign(where.teStartedAt || {}, { [Op.lte]: req.query.to }); + if (Op && toDate) { + where.teStartedAt = Object.assign(where.teStartedAt || {}, { [Op.lte]: toDate }); } const requestedLimit = parseInt(req.query.limit, 10); @@ -400,11 +421,15 @@ exports.exportCsv = async (req, res) => { where.teCustId = customerId; } const Op = db.Sequelize && db.Sequelize.Op; - if (Op && req.query.from) { - where.teStartedAt = Object.assign(where.teStartedAt || {}, { [Op.gte]: req.query.from }); + // Same parseDateOrNull guard as listByCompany — silent-drop on + // bad input rather than 500 from Postgres. + const fromDate = parseDateOrNull(req.query.from); + const toDate = parseDateOrNull(req.query.to); + if (Op && fromDate) { + where.teStartedAt = Object.assign(where.teStartedAt || {}, { [Op.gte]: fromDate }); } - if (Op && req.query.to) { - where.teStartedAt = Object.assign(where.teStartedAt || {}, { [Op.lte]: req.query.to }); + if (Op && toDate) { + where.teStartedAt = Object.assign(where.teStartedAt || {}, { [Op.lte]: toDate }); } const HARD_CAP = 5000; @@ -460,4 +485,4 @@ exports.exportCsv = async (req, res) => { }; // Exposed for unit testing. -exports._internals = { computeMinutes, isInvertedRange, IsMaster, GetCompanyId }; +exports._internals = { computeMinutes, isInvertedRange, parseDateOrNull, IsMaster, GetCompanyId }; diff --git a/tests/api/timeentry.test.js b/tests/api/timeentry.test.js index f3f9493..5e12e10 100644 --- a/tests/api/timeentry.test.js +++ b/tests/api/timeentry.test.js @@ -235,6 +235,45 @@ describe('isInvertedRange helper', () => { }); }); +describe('parseDateOrNull helper', () => { + // listByCompany + exportCsv use this on ?from / ?to query + // params. Before this guard, a `?from=not-a-date` plugged the + // string straight into Sequelize's Op.gte, which Postgres + // refused to parse as a timestamp — surfacing as a 500 from + // the controller's catch. The doc comment on both handlers + // promised "bad dates are silently dropped"; the helper makes + // that comment match the behavior. + let parseDateOrNull; + beforeAll(async () => { + const ctrl = await import('../../app/controllers/timeentrycontroller.js'); + parseDateOrNull = ctrl._internals.parseDateOrNull; + }); + + test('returns a Date for a valid ISO 8601 input', () => { + const d = parseDateOrNull('2026-05-15T10:00:00Z'); + expect(d).toBeInstanceOf(Date); + expect(d.toISOString()).toBe('2026-05-15T10:00:00.000Z'); + }); + + test('returns a Date for a valid date-only input', () => { + const d = parseDateOrNull('2026-05-15'); + expect(d).toBeInstanceOf(Date); + expect(Number.isFinite(d.getTime())).toBe(true); + }); + + test('returns null for garbage strings (the silent-drop case)', () => { + expect(parseDateOrNull('not-a-date')).toBeNull(); + expect(parseDateOrNull('totally-bogus')).toBeNull(); + }); + + test('returns null for empty / non-string input', () => { + expect(parseDateOrNull('')).toBeNull(); + expect(parseDateOrNull(undefined)).toBeNull(); + expect(parseDateOrNull(null)).toBeNull(); + expect(parseDateOrNull(42)).toBeNull(); + }); +}); + describe('DELETE /v1/timeentry/:id auth contract', () => { test('returns 403 when authKey header is missing', async () => { const res = await request(app).delete('/v1/timeentry/1');