From f13ec89beba84a63d9d9d950f82ffd990ccdf475 Mon Sep 17 00:00:00 2001 From: Groom Book CTO Date: Tue, 17 Mar 2026 19:30:25 +0000 Subject: [PATCH] fix: appointment conflict detection, soft-delete, and auth guardrail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes five bugs flagged in CEO code review (GitHub issues #18–22): - #18: Wrap conflict check + insert/update in a DB transaction to prevent double-booking race conditions under concurrent load. - #19: PATCH conflict detection now falls back to the existing appointment's staffId when staffId is omitted from the request body, so rescheduling always checks for conflicts. - #20: DELETE endpoint now soft-deletes (status = 'cancelled') instead of hard-deleting, preserving audit trail and financial records. - #21: Staff DELETE checks for existing non-cancelled appointments before deleting and returns 409 if any are found, preventing orphaned references. - #22: AUTH_DISABLED=true now logs a startup warning in development and calls process.exit(1) in production, preventing accidental auth bypass in deployed environments. Co-Authored-By: Paperclip --- apps/api/src/middleware/auth.ts | 15 +++ apps/api/src/routes/appointments.ts | 184 ++++++++++++++++++---------- apps/api/src/routes/staff.ts | 28 ++++- 3 files changed, 163 insertions(+), 64 deletions(-) diff --git a/apps/api/src/middleware/auth.ts b/apps/api/src/middleware/auth.ts index 82d7683..f8d6380 100644 --- a/apps/api/src/middleware/auth.ts +++ b/apps/api/src/middleware/auth.ts @@ -23,6 +23,21 @@ export interface JwtPayload { name?: string; } +// Guard: refuse to start with AUTH_DISABLED in production (fixes #22). +if (process.env.AUTH_DISABLED === "true") { + if (process.env.NODE_ENV === "production") { + console.error( + "[FATAL] AUTH_DISABLED=true is not allowed in production. " + + "Remove AUTH_DISABLED from your environment and configure OIDC_ISSUER." + ); + process.exit(1); + } + console.warn( + "[WARNING] AUTH_DISABLED=true — authentication is bypassed. " + + "Do NOT use this in production." + ); +} + export const authMiddleware: MiddlewareHandler = async (c, next) => { if (process.env.AUTH_DISABLED === "true") { c.set("jwtPayload", { sub: "dev-user" } as JwtPayload); diff --git a/apps/api/src/routes/appointments.ts b/apps/api/src/routes/appointments.ts index 05f87cc..9859d69 100644 --- a/apps/api/src/routes/appointments.ts +++ b/apps/api/src/routes/appointments.ts @@ -34,32 +34,6 @@ const updateAppointmentSchema = z.object({ priceCents: z.number().int().positive().nullable().optional(), }); -/** Returns true if a staff member has a non-cancelled appointment overlapping [start, end). */ -async function hasConflict( - staffId: string, - start: Date, - end: Date, - excludeId?: string -): Promise { - const db = getDb(); - const conditions = [ - eq(appointments.staffId, staffId), - // Overlap: existing.start < end AND existing.end > start - lt(appointments.startTime, end), - gte(appointments.endTime, start), - // Ignore cancelled/no_show - ne(appointments.status, "cancelled"), - ne(appointments.status, "no_show"), - ]; - if (excludeId) conditions.push(ne(appointments.id, excludeId)); - const rows = await db - .select({ id: appointments.id }) - .from(appointments) - .where(and(...conditions)) - .limit(1); - return rows.length > 0; -} - // List appointments, optionally filtered by date range or staffId appointmentsRouter.get("/", async (c) => { const db = getDb(); @@ -110,20 +84,49 @@ appointmentsRouter.post( return c.json({ error: "endTime must be after startTime" }, 422); } - if (body.staffId) { - const conflict = await hasConflict(body.staffId, start, end); - if (conflict) { + // Wrap conflict check + insert in a transaction to prevent double-booking + // race conditions under concurrent load (fixes #18). + let row; + try { + row = await db.transaction(async (tx) => { + if (body.staffId) { + const conflicts = await tx + .select({ id: appointments.id }) + .from(appointments) + .where( + and( + eq(appointments.staffId, body.staffId), + lt(appointments.startTime, end), + gte(appointments.endTime, start), + ne(appointments.status, "cancelled"), + ne(appointments.status, "no_show"), + ) + ) + .limit(1); + if (conflicts.length > 0) { + throw Object.assign(new Error("conflict"), { statusCode: 409 }); + } + } + + const [inserted] = await tx + .insert(appointments) + .values({ ...body, startTime: start, endTime: end }) + .returning(); + return inserted; + }); + } catch (err: unknown) { + if ( + err instanceof Error && + (err as Error & { statusCode?: number }).statusCode === 409 + ) { return c.json( { error: "Staff member has a conflicting appointment at this time" }, 409 ); } + throw err; } - const [row] = await db - .insert(appointments) - .values({ ...body, startTime: start, endTime: end }) - .returning(); return c.json(row, 201); } ); @@ -136,38 +139,92 @@ appointmentsRouter.patch( const id = c.req.param("id"); const body = c.req.valid("json"); - // If rescheduling, check for conflicts - if ((body.startTime || body.endTime || body.staffId !== undefined) && body.staffId) { - const existing = await db - .select() - .from(appointments) - .where(eq(appointments.id, id)) - .limit(1); - const current = existing[0]; - if (!current) return c.json({ error: "Not found" }, 404); - - const start = body.startTime ? new Date(body.startTime) : current.startTime; - const end = body.endTime ? new Date(body.endTime) : current.endTime; - const staffId = body.staffId ?? current.staffId; - - if (end <= start) { - return c.json({ error: "endTime must be after startTime" }, 422); - } - - if (staffId) { - const conflict = await hasConflict(staffId, start, end, id); - if (conflict) { - return c.json( - { error: "Staff member has a conflicting appointment at this time" }, - 409 - ); - } - } - } + const needsConflictCheck = + body.startTime !== undefined || + body.endTime !== undefined || + body.staffId !== undefined; const update: Record = { ...body, updatedAt: new Date() }; if (body.startTime) update.startTime = new Date(body.startTime); if (body.endTime) update.endTime = new Date(body.endTime); + + if (needsConflictCheck) { + // Wrap conflict check + update in a transaction to prevent race conditions + // (fixes #18). Also falls back to the existing staffId when staffId is + // omitted from the request, so rescheduling always checks conflicts (fixes #19). + let row; + try { + row = await db.transaction(async (tx) => { + const [current] = await tx + .select() + .from(appointments) + .where(eq(appointments.id, id)) + .limit(1); + if (!current) { + throw Object.assign(new Error("not found"), { statusCode: 404 }); + } + + const start = body.startTime + ? new Date(body.startTime) + : current.startTime; + const end = body.endTime ? new Date(body.endTime) : current.endTime; + // Use provided staffId (may be null to unassign); fall back to existing + const staffId = + body.staffId !== undefined ? body.staffId : current.staffId; + + if (end <= start) { + throw Object.assign(new Error("end before start"), { + statusCode: 422, + }); + } + + if (staffId) { + const conflicts = await tx + .select({ id: appointments.id }) + .from(appointments) + .where( + and( + eq(appointments.staffId, staffId), + lt(appointments.startTime, end), + gte(appointments.endTime, start), + ne(appointments.status, "cancelled"), + ne(appointments.status, "no_show"), + ne(appointments.id, id), + ) + ) + .limit(1); + if (conflicts.length > 0) { + throw Object.assign(new Error("conflict"), { statusCode: 409 }); + } + } + + const [updated] = await tx + .update(appointments) + .set(update) + .where(eq(appointments.id, id)) + .returning(); + return updated; + }); + } catch (err: unknown) { + const statusCode = (err as Error & { statusCode?: number }).statusCode; + if (statusCode === 404) return c.json({ error: "Not found" }, 404); + if (statusCode === 422) + return c.json({ error: "endTime must be after startTime" }, 422); + if (statusCode === 409) + return c.json( + { + error: + "Staff member has a conflicting appointment at this time", + }, + 409 + ); + throw err; + } + + if (!row) return c.json({ error: "Not found" }, 404); + return c.json(row); + } + const [row] = await db .update(appointments) .set(update) @@ -178,10 +235,13 @@ appointmentsRouter.patch( } ); +// Soft-delete: cancel the appointment instead of removing the row, +// preserving audit trail and financial records (fixes #20). appointmentsRouter.delete("/:id", async (c) => { const db = getDb(); const [row] = await db - .delete(appointments) + .update(appointments) + .set({ status: "cancelled", updatedAt: new Date() }) .where(eq(appointments.id, c.req.param("id"))) .returning(); if (!row) return c.json({ error: "Not found" }, 404); diff --git a/apps/api/src/routes/staff.ts b/apps/api/src/routes/staff.ts index b305735..60e31dc 100644 --- a/apps/api/src/routes/staff.ts +++ b/apps/api/src/routes/staff.ts @@ -1,7 +1,7 @@ import { Hono } from "hono"; import { zValidator } from "@hono/zod-validator"; import { z } from "zod"; -import { eq, getDb, staff } from "@groombook/db"; +import { and, eq, getDb, ne, staff, appointments } from "@groombook/db"; export const staffRouter = new Hono(); @@ -55,9 +55,33 @@ staffRouter.patch("/:id", zValidator("json", updateStaffSchema), async (c) => { staffRouter.delete("/:id", async (c) => { const db = getDb(); + const id = c.req.param("id"); + + // Prevent deleting staff who have existing non-cancelled appointments (fixes #21). + const activeAppointments = await db + .select({ id: appointments.id }) + .from(appointments) + .where( + and( + eq(appointments.staffId, id), + ne(appointments.status, "cancelled"), + ne(appointments.status, "no_show"), + ) + ) + .limit(1); + if (activeAppointments.length > 0) { + return c.json( + { + error: + "Cannot delete staff member with existing appointments. Reassign or cancel their appointments first.", + }, + 409 + ); + } + const [row] = await db .delete(staff) - .where(eq(staff.id, c.req.param("id"))) + .where(eq(staff.id, id)) .returning(); if (!row) return c.json({ error: "Not found" }, 404); return c.json({ ok: true }); -- 2.52.0