From 7465852402bbf8964c5618b7d47b0dc7922b00ac Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Tue, 19 May 2026 01:00:09 -0500 Subject: [PATCH] feat(timeentry): reject inverted teStartedAt/teEndedAt at the schema layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `computeMinutes` in `timeentrycontroller.js` already maps an inverted date range (`teEndedAt < teStartedAt`) to `teMinutes = null` and keeps going — the user gets a 201 with `teMinutes: null` and no signal that their request was nonsense. The DB ends up with a time entry whose duration is undefined and whose bounds say "I clocked out before I clocked in." That's operator error worth flagging at the boundary instead of papering over. Add a zod `.refine()` cross-field check on both `createTimeEntryBody` and `updateTimeEntryBody`. CREATE always sees both bounds when `teEndedAt` is supplied, so the refinement fires whenever the bug is present. PATCH only fires when both bounds are in the same request body — the single-bound PATCH case (only `teEndedAt`, where the existing `teStartedAt` lives in the DB) can't be validated at the schema layer without a DB read; that path is a follow-up controller-level tightening and is intentionally still accepted here so the schema stays pure. Equality (zero-minute entry) is allowed: someone clocking a meeting that ran exactly 0 minutes is a valid edge case, not a bug. Five new tests cover: inverted CREATE → 400, equality CREATE → schema-pass, in-flight CREATE (no teEndedAt) → schema-pass, inverted both-bound PATCH → 400, single-bound PATCH → schema-pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- app/schemas/timeentry.schema.js | 30 ++++++++++++- tests/api/timeentry.test.js | 76 +++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 2 deletions(-) diff --git a/app/schemas/timeentry.schema.js b/app/schemas/timeentry.schema.js index ab041a5..32550b0 100644 --- a/app/schemas/timeentry.schema.js +++ b/app/schemas/timeentry.schema.js @@ -21,6 +21,12 @@ const isoDatetime = z.string().datetime({ * * Server-managed fields (teId, teMinutes, teArch) are not accepted * from the body. + * + * Cross-field refinement: when both teStartedAt and teEndedAt are + * present, teEndedAt must be at or after teStartedAt. The controller + * already maps an inverted range to `teMinutes = null` silently, but + * the user has no signal that their request was nonsense — they + * still get a 201 with bad data. Reject at the boundary instead. */ const createTimeEntryBody = z.object({ teCustId: z.coerce.number().int().positive(), @@ -31,13 +37,26 @@ const createTimeEntryBody = z.object({ teBillable: z.boolean().optional(), }).strict({ message: 'Unexpected field in body. Whitelist: teCustId, teCompId, teDescription, teStartedAt, teEndedAt, teBillable.', -}); +}).refine( + (data) => !data.teEndedAt || new Date(data.teEndedAt) >= new Date(data.teStartedAt), + { + message: 'teEndedAt must be at or after teStartedAt.', + path: ['teEndedAt'], + }, +); /** * PATCH /v1/timeentry/:id body. None of the fields are required — * a PATCH is a partial update — but at least one must be present. * The controller already handles the "no updatable fields" case * with a 400; the schema just enforces shape. + * + * Cross-field refinement: when both teStartedAt and teEndedAt are + * present in the same PATCH, the same end >= start constraint as + * CREATE applies. (The single-field PATCH case — updating only one + * bound against the DB's existing value — has to be enforced at + * the controller layer because the schema doesn't see the existing + * row; controller already returns `teMinutes = null` there.) */ const updateTimeEntryBody = z.object({ teDescription: z.string().max(10000).optional(), @@ -46,7 +65,14 @@ const updateTimeEntryBody = z.object({ teBillable: z.boolean().optional(), }).strict({ message: 'Unexpected field in body. Whitelist: teDescription, teStartedAt, teEndedAt, teBillable.', -}); +}).refine( + (data) => !(data.teStartedAt && data.teEndedAt) || + new Date(data.teEndedAt) >= new Date(data.teStartedAt), + { + message: 'teEndedAt must be at or after teStartedAt.', + path: ['teEndedAt'], + }, +); /** * GET /v1/timeentry/export.csv query schema. Like listByCompanyQuery diff --git a/tests/api/timeentry.test.js b/tests/api/timeentry.test.js index 242514c..2618979 100644 --- a/tests/api/timeentry.test.js +++ b/tests/api/timeentry.test.js @@ -83,6 +83,82 @@ describe('POST /v1/timeentry auth contract', () => { }); }); +describe('POST /v1/timeentry body validation', () => { + test('400 when teEndedAt is strictly before teStartedAt', async () => { + // Inverted range. Schema runs before auth — this should 400 + // with a refinement issue on teEndedAt, never reaching the + // controller where the silent `teMinutes = null` fallback + // would otherwise accept the bad write. + const res = await request(app) + .post('/v1/timeentry') + .send({ + teCustId: 1, + teStartedAt: '2026-05-16T10:00:00Z', + teEndedAt: '2026-05-16T09:00:00Z', + }); + expect(res.status).toBe(400); + expect(res.body.issues).toBeDefined(); + const issue = res.body.issues.find((i) => i.path === 'teEndedAt'); + expect(issue).toBeDefined(); + expect(issue.message).toMatch(/at or after teStartedAt/i); + }); + + test('201-path (schema passes) when teEndedAt equals teStartedAt', async () => { + // Exact equality is allowed: a zero-minute entry is a valid + // edge case (someone clocking a meeting that ran 0 minutes). + // Schema accepts; auth/controller is what decides the final + // status — without an authKey it'll 403, but the point is + // the schema didn't 400. + const res = await request(app) + .post('/v1/timeentry') + .send({ + teCustId: 1, + teStartedAt: '2026-05-16T09:00:00Z', + teEndedAt: '2026-05-16T09:00:00Z', + }); + expect(res.status).not.toBe(400); + }); + + test('passes when teEndedAt is omitted (in-flight entry)', async () => { + // Open-ended entry is the canonical "clock in but not out yet" + // path. Refinement must not fire when teEndedAt is undefined. + const res = await request(app) + .post('/v1/timeentry') + .send({ + teCustId: 1, + teStartedAt: '2026-05-16T09:00:00Z', + }); + expect(res.status).not.toBe(400); + }); +}); + +describe('PATCH /v1/timeentry/:id body validation', () => { + test('400 when both bounds are sent and teEndedAt is before teStartedAt', async () => { + const res = await request(app) + .patch('/v1/timeentry/1') + .send({ + teStartedAt: '2026-05-16T10:00:00Z', + teEndedAt: '2026-05-16T09:00:00Z', + }); + expect(res.status).toBe(400); + expect(res.body.issues).toBeDefined(); + const issue = res.body.issues.find((i) => i.path === 'teEndedAt'); + expect(issue).toBeDefined(); + }); + + test('single-bound PATCH (only teEndedAt) is not blocked by the schema', async () => { + // The cross-field refinement can't validate a single-bound + // PATCH without seeing the existing row, so the schema must + // not reject it; the controller's `computeMinutes` is the + // only thing that sees the merged value. (Tightening that + // path is a separate, controller-layer change.) + const res = await request(app) + .patch('/v1/timeentry/1') + .send({ teEndedAt: '2026-05-16T09:00:00Z' }); + expect(res.status).not.toBe(400); + }); +}); + describe('GET /v1/timeentry/:id auth contract', () => { test('returns 403 when authKey header is missing', async () => { const res = await request(app).get('/v1/timeentry/1');