diff --git a/app/controllers/timeentrycontroller.js b/app/controllers/timeentrycontroller.js index 5baf07a..d05badc 100644 --- a/app/controllers/timeentrycontroller.js +++ b/app/controllers/timeentrycontroller.js @@ -30,6 +30,29 @@ function computeMinutes(startedAt, endedAt) { return Math.round((end - start) / 60000); } +/** + * Return true when both bounds are non-empty AND parse cleanly AND + * endedAt is strictly before startedAt. Used by the PATCH handler to + * reject single-bound updates whose merged interval would invert. + * + * The schema-layer refinement on updateTimeEntryBody (#130) catches + * the both-bounds-in-body case; this helper covers the half the + * schema can't see — a PATCH that supplies only one of the two + * bounds and merges against the row's existing value. + * + * Equality is NOT inverted (zero-minute entries are legitimate). + * Unparseable input is NOT inverted (computeMinutes will surface + * the null-result via its own NaN guard; flagging it here would + * be a false positive on garbage we'd 400 elsewhere anyway). + */ +function isInvertedRange(startedAt, endedAt) { + if (!startedAt || !endedAt) return false; + const start = new Date(startedAt).getTime(); + const end = new Date(endedAt).getTime(); + if (!Number.isFinite(start) || !Number.isFinite(end)) return false; + return end < start; +} + /** * POST /v1/timeentry * @@ -236,10 +259,25 @@ exports.update = async (req, res) => { } // Recompute minutes if either bound changed. if (updates.teStartedAt !== undefined || updates.teEndedAt !== undefined) { - updates.teMinutes = computeMinutes( - updates.teStartedAt !== undefined ? updates.teStartedAt : entry.teStartedAt, - updates.teEndedAt !== undefined ? updates.teEndedAt : entry.teEndedAt, - ); + const mergedStart = updates.teStartedAt !== undefined + ? updates.teStartedAt : entry.teStartedAt; + const mergedEnd = updates.teEndedAt !== undefined + ? updates.teEndedAt : entry.teEndedAt; + // Inverted-range guard for the single-bound PATCH case. The + // schema-layer refinement in updateTimeEntryBody (#130) only + // sees fields present in the request body, so a PATCH that + // supplies only one bound can't be validated there — the + // row's existing value lives in the DB. Reject merged + // end < start at 400 instead of silently dropping `teMinutes` + // to null and storing a row whose clocked-out-before-clocked- + // in timestamps look correct but whose duration column is + // blank. + if (isInvertedRange(mergedStart, mergedEnd)) { + return res.status(400).json({ + message: 'teEndedAt must be at or after teStartedAt.', + }); + } + updates.teMinutes = computeMinutes(mergedStart, mergedEnd); } try { await entry.update(updates); @@ -414,4 +452,4 @@ exports.exportCsv = async (req, res) => { }; // Exposed for unit testing. -exports._internals = { computeMinutes, IsMaster, GetCompanyId }; +exports._internals = { computeMinutes, isInvertedRange, IsMaster, GetCompanyId }; diff --git a/tests/api/timeentry.test.js b/tests/api/timeentry.test.js index 2618979..600cab0 100644 --- a/tests/api/timeentry.test.js +++ b/tests/api/timeentry.test.js @@ -187,6 +187,54 @@ describe('PATCH /v1/timeentry/:id auth contract', () => { }); }); +describe('isInvertedRange helper', () => { + // The PATCH /v1/timeentry/:id handler uses this to reject the + // single-bound update case where the schema-layer refinement + // can't see the row's existing other bound. Returning true means + // "reject with 400"; false means "let it through". + // + // Unit-tested in isolation rather than via supertest because the + // full PATCH path requires auth + DB mocks that don't compose + // cleanly with vitest's per-file vi.mock model for this codebase. + // The controller is a 1-line `if (isInvertedRange(a, b)) return + // res.status(400)...`, so a unit test on the helper is the + // tightest coverage. + let isInvertedRange; + beforeAll(async () => { + const ctrl = await import('../../app/controllers/timeentrycontroller.js'); + isInvertedRange = ctrl._internals.isInvertedRange; + }); + + test('false when either bound is missing (open-ended entry, legitimate)', () => { + expect(isInvertedRange(null, '2026-05-15T10:00:00Z')).toBe(false); + expect(isInvertedRange('2026-05-15T10:00:00Z', null)).toBe(false); + expect(isInvertedRange(null, null)).toBe(false); + expect(isInvertedRange(undefined, undefined)).toBe(false); + }); + + test('false when ended >= started (the happy paths)', () => { + expect(isInvertedRange('2026-05-15T09:00:00Z', '2026-05-15T10:00:00Z')).toBe(false); + // Equality: zero-minute entry is legitimate. + expect(isInvertedRange('2026-05-15T09:00:00Z', '2026-05-15T09:00:00Z')).toBe(false); + }); + + test('true when ended < started (the bug we reject at 400)', () => { + expect(isInvertedRange('2026-05-15T10:00:00Z', '2026-05-15T09:00:00Z')).toBe(true); + // Even one millisecond difference flips the bit. + expect(isInvertedRange('2026-05-15T10:00:00.001Z', '2026-05-15T10:00:00.000Z')).toBe(true); + }); + + test('false on unparseable input (computeMinutes guards null elsewhere)', () => { + // The helper deliberately does NOT 400 on garbage strings — those + // get null teMinutes via computeMinutes' NaN guard. Treating them + // as "inverted" here would be a false positive flagging input + // that's already otherwise-broken. + expect(isInvertedRange('not-a-date', '2026-05-15T10:00:00Z')).toBe(false); + expect(isInvertedRange('2026-05-15T10:00:00Z', 'also-not-a-date')).toBe(false); + expect(isInvertedRange('garbage', 'garbage')).toBe(false); + }); +}); + 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');