diff --git a/src/routes/health.ts b/src/routes/health.ts index 8a430de2..630aaac3 100644 --- a/src/routes/health.ts +++ b/src/routes/health.ts @@ -5,6 +5,7 @@ import { db } from "../db/client"; import { auditLogs } from "../db/schema"; import { eq, desc } from "drizzle-orm"; import { requestTimeout, abortableRace } from "../middleware/timeout"; +import { idempotency } from "../middleware/idempotency"; export const healthRouter = Router(); @@ -30,7 +31,11 @@ healthRouter.get("/", async (_req, res, next) => { } }); -healthRouter.post("/mutations", async (req, res, next) => { +// The global Idempotency-Key middleware (src/index.ts) is registered after +// this router's /api/health mount, so it never runs for this route; applying +// it here directly makes retried mutations (e.g. after a client-side timeout) +// safe without double-writing audit log entries. +healthRouter.post("/mutations", idempotency, async (req, res, next) => { try { const ip = req.ip || req.socket.remoteAddress || "unknown"; const correlationId = getRequestId(); diff --git a/tests/healthIdempotency.test.ts b/tests/healthIdempotency.test.ts new file mode 100644 index 00000000..7ba0c7ee --- /dev/null +++ b/tests/healthIdempotency.test.ts @@ -0,0 +1,172 @@ +/** + * tests/healthIdempotency.test.ts + * + * Verifies POST /api/health/mutations is protected by the Idempotency-Key + * middleware (issue #665). The global idempotency middleware in src/index.ts + * is registered *after* /api/health, so it never runs for this route; + * src/routes/health.ts now applies it directly on the /mutations route. + * + * The db mock is a small stateful in-memory map (mirrors the approach in + * tests/authIdempotency.test.ts) so persist-then-replay round trips can be + * exercised without a real database. + */ + +import { auditLogs, idempotencyRecords } from "../src/db/schema"; + +const idempotencyStore = new Map>(); + +// src/middleware/timeout.ts's requestTimeout() aborts res.locals.abortSignal +// on the request's "close" event, which — independent of this change — can +// fire under supertest before the response is fully sent, racing +// abortableRace() in the health route handler. That's a pre-existing, +// unrelated issue (reproduces with requestTimeout + abortableRace alone, no +// idempotency involved); stub both out here so this suite stays focused on +// idempotency behavior specifically. +jest.mock("../src/middleware/timeout", () => ({ + requestTimeout: () => (_req: unknown, _res: unknown, next: () => void) => next(), + abortableRace: (promise: Promise) => promise, +})); + +// src/middleware/idempotency.ts reads/writes via "../db" (src/db/index.ts) — +// a separate module/instance from "../db/client", which the health route +// itself uses for its own auditLogs query below. +jest.mock("../src/db", () => ({ + db: { + select: () => ({ + from: () => ({ + where: () => ({ + limit: async () => Array.from(idempotencyStore.values()), + }), + }), + }), + insert: () => ({ + values: async (record: Record) => { + idempotencyStore.set(record.key as string, record); + }, + }), + }, +})); + +jest.mock("../src/db/client", () => ({ + db: { + select: () => ({ + from: () => ({ + where: () => ({ + orderBy: () => ({ + limit: async () => [ + { afterState: { mode: "active", maintenance: false } }, + ], + }), + }), + }), + }), + }, + pool: { query: jest.fn() }, +})); + +jest.mock("../src/services/auditService", () => ({ + createAuditLog: jest.fn().mockResolvedValue("mock-correlation-id"), +})); + +import express from "express"; +import request from "supertest"; +import { healthRouter } from "../src/routes/health"; +import { errorHandler } from "../src/middleware/errorHandler"; +import { createAuditLog } from "../src/services/auditService"; + +const mockCreateAuditLog = createAuditLog as jest.MockedFunction; + +function makeApp(): express.Express { + const app = express(); + app.use(express.json()); + app.use("/api/health", healthRouter); + app.use(errorHandler); + return app; +} + +let app: express.Express; + +beforeEach(() => { + jest.clearAllMocks(); + idempotencyStore.clear(); + app = makeApp(); +}); + +describe("Idempotency for POST /api/health/mutations", () => { + it("replays the stored response for a repeated Idempotency-Key + body", async () => { + const key = "health-mutation-key-1"; + const body = { mode: "maintenance", maintenance: true }; + + const first = await request(app) + .post("/api/health/mutations") + .set("Idempotency-Key", key) + .send(body); + + expect(first.status).toBe(200); + expect(first.headers["idempotent-replayed"]).toBeUndefined(); + expect(first.body.status).toBe("updated"); + expect(mockCreateAuditLog).toHaveBeenCalledTimes(1); + + const second = await request(app) + .post("/api/health/mutations") + .set("Idempotency-Key", key) + .send(body); + + expect(second.status).toBe(200); + expect(second.headers["idempotent-replayed"]).toBe("true"); + expect(second.body).toEqual(first.body); + // The route handler (and its audit log write) must not run a second time. + expect(mockCreateAuditLog).toHaveBeenCalledTimes(1); + }); + + it("returns 409 when the same Idempotency-Key is reused with a different body", async () => { + const key = "health-mutation-key-2"; + + await request(app) + .post("/api/health/mutations") + .set("Idempotency-Key", key) + .send({ mode: "maintenance", maintenance: true }) + .expect(200); + + const conflict = await request(app) + .post("/api/health/mutations") + .set("Idempotency-Key", key) + .send({ mode: "active", maintenance: false }) + .expect(409); + + expect(conflict.body.error.code).toBe("conflict"); + expect(mockCreateAuditLog).toHaveBeenCalledTimes(1); + }); + + it("returns 400 for a malformed Idempotency-Key", async () => { + const res = await request(app) + .post("/api/health/mutations") + .set("Idempotency-Key", "a".repeat(256)) + .send({ mode: "maintenance", maintenance: true }); + + expect(res.status).toBe(400); + expect(res.body.error.code).toBe("invalid_idempotency_key"); + expect(mockCreateAuditLog).not.toHaveBeenCalled(); + }); + + it("processes every request independently when no Idempotency-Key is sent", async () => { + await request(app) + .post("/api/health/mutations") + .send({ mode: "maintenance", maintenance: true }) + .expect(200); + + await request(app) + .post("/api/health/mutations") + .send({ mode: "maintenance", maintenance: true }) + .expect(200); + + expect(mockCreateAuditLog).toHaveBeenCalledTimes(2); + }); +}); + +// Sanity check that the schema imports used to build the mock actually exist. +describe("schema sanity", () => { + it("idempotencyRecords and auditLogs are distinct table objects", () => { + expect(idempotencyRecords).not.toBe(auditLogs); + }); +});