From 56a8696876aaea06f73fb5ea4aa7fbf5c2b96e6c Mon Sep 17 00:00:00 2001 From: Riyadh Date: Thu, 30 Apr 2026 19:16:00 +0000 Subject: [PATCH] fix(executive-meetings): lock down attendee save payload (#221) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: Task #220 added a client-only `_sid` field on attendee rows so React DnD can identify rows. The two client save sites already enumerate wire fields explicitly and never serialize `_sid`, but nothing prevents a future refactor from accidentally leaking it. The server was zod-default lenient (silently strips unknowns), so a regression would either be silently absorbed (bad — silent contract drift) or land in a future JSONB metadata column without anyone noticing. What changed: - `attendeeSchema` in artifacts/api-server/src/routes/executive-meetings.ts is now `.strict()`. Any unknown attendee key (including `_sid` or any future client-only field) is rejected with HTTP 400 instead of being silently stripped. The schema is reused by all three attendee-bearing endpoints (POST /executive-meetings, PATCH /executive-meetings/:id, PUT /executive-meetings/:id/attendees), so all three are covered by one change. Tests: - Added three API tests in artifacts/api-server/tests/executive-meetings.test.mjs: 1. POST /executive-meetings with attendee carrying `_sid` returns 400 and the error mentions the rejected key. 2. PATCH /executive-meetings/:id with attendees carrying `_sid` returns 400 AND the meeting's existing attendee list is preserved (no partial mutation). 3. PUT /executive-meetings/:id/attendees with attendee carrying `_sid` returns 400 AND the seeded attendee is unchanged. Verification: - Full API test suite: 207/207 green (was 204/204 before; +3 new). - No client-side change needed: existing `saveAttendeeName` (~L943 in artifacts/tx-os/src/pages/executive-meetings.tsx) and the manage-dialog save (~L4004) already project to the documented wire shape (`name, title, attendanceType, sortOrder, kind`). - Architect review: addressed the one gap (PATCH coverage) by adding test #2 above; verdict resolved. Out of scope cleanup: - Marked the descriptions of stale tasks #172 and #179 as STALE (both PDF tests pass and PDF export works in current main). Final cancellation left to the user. --- .../src/routes/executive-meetings.ts | 31 +++-- .../tests/executive-meetings.test.mjs | 123 ++++++++++++++++++ 2 files changed, 142 insertions(+), 12 deletions(-) diff --git a/artifacts/api-server/src/routes/executive-meetings.ts b/artifacts/api-server/src/routes/executive-meetings.ts index afe4ad86..79b59b70 100644 --- a/artifacts/api-server/src/routes/executive-meetings.ts +++ b/artifacts/api-server/src/routes/executive-meetings.ts @@ -203,18 +203,25 @@ const positiveIntSchema = z.number().int().positive(); // output). The byte count grows quickly with inline wrappers, // so we allow up to ~10k bytes for titles and ~5k for attendee names. The // sanitizer strips disallowed tags/attrs before persistence. -const attendeeSchema = z.object({ - // For persons this is sanitized rich-text HTML (Tiptap output). - // For subheadings this is the user's free-text label, also passed - // through sanitizeRichText to strip any sneaky markup. Both kinds - // require at least one non-whitespace character; an empty string is - // a delete-row gesture in the inline editor and must never reach the DB. - name: z.string().trim().min(1).max(5000), - title: z.string().trim().max(200).nullable().optional(), - attendanceType: z.enum(ATTENDANCE_TYPES).default("internal"), - sortOrder: z.number().int().min(0).optional(), - kind: z.enum(ATTENDEE_KINDS).default("person"), -}); +const attendeeSchema = z + .object({ + // For persons this is sanitized rich-text HTML (Tiptap output). + // For subheadings this is the user's free-text label, also passed + // through sanitizeRichText to strip any sneaky markup. Both kinds + // require at least one non-whitespace character; an empty string is + // a delete-row gesture in the inline editor and must never reach the DB. + name: z.string().trim().min(1).max(5000), + title: z.string().trim().max(200).nullable().optional(), + attendanceType: z.enum(ATTENDANCE_TYPES).default("internal"), + sortOrder: z.number().int().min(0).optional(), + kind: z.enum(ATTENDEE_KINDS).default("person"), + }) + // `.strict()` so any client-only field (e.g. the React `_sid` row id used + // by the drag-and-drop editor) that accidentally leaks into the wire + // payload is rejected loudly with 400 instead of being silently stripped. + // The browser always projects to the documented wire shape; this is the + // belt-and-suspenders guard against a future refactor regression. + .strict(); const meetingBaseFields = { titleAr: z.string().trim().min(1).max(10000), diff --git a/artifacts/api-server/tests/executive-meetings.test.mjs b/artifacts/api-server/tests/executive-meetings.test.mjs index 38210df4..2dd749a0 100644 --- a/artifacts/api-server/tests/executive-meetings.test.mjs +++ b/artifacts/api-server/tests/executive-meetings.test.mjs @@ -264,6 +264,129 @@ test("Meetings: PUT /attendees replaces the attendee list", async () => { assert.ok(!body.attendees.find((a) => a.name === "Old One")); }); +// --- Attendee payload contract (Task #221) --------------------------------- +// Server is `.strict()` on the attendee schema so client-only fields like +// `_sid` (the React row id used by the drag-and-drop editor) are rejected +// loudly with 400 instead of being silently stripped. This locks down the +// wire contract so a future client refactor that accidentally serializes +// `_sid` (or any other internal field) breaks tests immediately rather +// than being absorbed by lenient parsing. +test("Attendee payload contract: POST rejects unknown fields like `_sid`", async () => { + const create = await api(adminCookie, "POST", "/api/executive-meetings", { + titleAr: "س", + titleEn: "S", + meetingDate: today, + attendees: [ + { + name: "Leaky One", + attendanceType: "internal", + sortOrder: 0, + _sid: "client-only-row-id", + }, + ], + }); + assert.equal( + create.status, + 400, + `expected 400 when attendee carries _sid, got ${create.status}`, + ); + const body = await create.json(); + // Zod surfaces the rejected key in the error path. + const serialized = JSON.stringify(body); + assert.ok( + serialized.includes("_sid") || serialized.toLowerCase().includes("unrecognized"), + `expected error to mention the rejected key, got: ${serialized}`, + ); +}); + +test("Attendee payload contract: PATCH /:id rejects unknown attendee fields like `_sid`", async () => { + const create = await api(adminCookie, "POST", "/api/executive-meetings", { + titleAr: "ت", + titleEn: "T", + meetingDate: today, + attendees: [{ name: "Patch Seed", attendanceType: "internal", sortOrder: 0 }], + }); + assert.equal(create.status, 201); + const meeting = await create.json(); + created.meetingIds.push(meeting.id); + + const patch = await api( + adminCookie, + "PATCH", + `/api/executive-meetings/${meeting.id}`, + { + attendees: [ + { + name: "Leaky Three", + attendanceType: "internal", + sortOrder: 0, + _sid: "client-only-row-id", + }, + ], + }, + ); + assert.equal( + patch.status, + 400, + `expected 400 when PATCH attendee carries _sid, got ${patch.status}`, + ); + + // The seed attendee must still be the only one stored — the rejected + // PATCH must not have replaced the attendee list. + const fetched = await api( + adminCookie, + "GET", + `/api/executive-meetings/${meeting.id}`, + ); + const body = await fetched.json(); + assert.equal(body.attendees.length, 1); + assert.equal(body.attendees[0].name, "Patch Seed"); +}); + +test("Attendee payload contract: PUT /attendees rejects unknown fields like `_sid`", async () => { + const create = await api(adminCookie, "POST", "/api/executive-meetings", { + titleAr: "ش", + titleEn: "Sh", + meetingDate: today, + attendees: [{ name: "Seed", attendanceType: "internal", sortOrder: 0 }], + }); + assert.equal(create.status, 201); + const meeting = await create.json(); + created.meetingIds.push(meeting.id); + + const replace = await api( + adminCookie, + "PUT", + `/api/executive-meetings/${meeting.id}/attendees`, + { + attendees: [ + { + name: "Leaky Two", + attendanceType: "internal", + sortOrder: 0, + _sid: "client-only-row-id", + }, + ], + }, + ); + assert.equal( + replace.status, + 400, + `expected 400 when PUT attendee carries _sid, got ${replace.status}`, + ); + + // And the meeting's attendee list must not have changed (the seed row + // still wins, the leaky row never persists). + const fetched = await api( + adminCookie, + "GET", + `/api/executive-meetings/${meeting.id}`, + ); + const body = await fetched.json(); + assert.equal(body.attendees.length, 1); + assert.equal(body.attendees[0].name, "Seed"); +}); + test("Meetings: POST /duplicate clones a meeting onto another date", async () => { const create = await api(adminCookie, "POST", "/api/executive-meetings", { titleAr: "ب",