EM #241: sanitize location/meetingUrl/notes (regex stripper) + tests

Original task: Executive Meetings — test coverage + sanitization closeout (umbrella for #170, #186-189, #201, #202, #212, #214, #218, #235). What landed - Added `stripTagsToPlainText[OrNull]` in `artifacts/api-server/src/lib/sanitize.ts`. Implementation is a two-pass regex stripper (NOT sanitize-html + entity decode): Pass 1: drop dangerous tag bodies entirely (`<script>`/`<style>`/`<noscript>`/`<iframe>`/`<object>`/ `<embed>`/`<template>` content + tags). Pass 2: strip HTML comments, CDATA, DOCTYPE, processing instructions, and any remaining open/close tags via `<\/?[a-zA-Z][^>]*>`. No entity decode pass — so URLs (`?a=1&b=2`) round-trip unchanged, plain text (`5 < 10`) is preserved, AND attacker-supplied encoded payloads (`<script>…`) survive as inert text instead of being rehydrated into live tags. The existing `sanitizePlainText` (which entity-encodes) is preserved for `attendee.title` so the print template's HTML interpolation behavior is unchanged. - Wired the new helper into all 11 write paths for `location`/`meetingUrl`/`notes` in `artifacts/api-server/src/routes/executive-meetings.ts`: POST /executive-meetings, PATCH /executive-meetings/:id, POST /executive-meetings/:id/duplicate, and `applyApprovedRequest` (`change_location` + `note`). attendee.title call sites kept as-is. - Added 6 API tests in `artifacts/api-server/tests/executive-meetings.test.mjs`: 1. POST sanitization (URL `&` round-trip + literal `<`/`&` in notes + asserts NO entity-encoding in stored values). 2. Encoded-payload regression guard (`<script>`, `<script>`, mixed-case `<ScRiPt>`/`<IFRAME>`) confirming we don't decode entities into live tags. 3. PATCH sanitization on the same fields. 4. Duplicate-path round-trip (URL ampersands preserved). 5. change_location approved-request round-trip + tag stripping. 6. EditableCell column-independence contract test (PATCH titleEn alone must not clobber titleAr and vice versa). Drift from the original umbrella - The umbrella also listed 9 Playwright e2e specs (#170, #186, #187, #188, #201, #212, #214, #218, #235). Each is a 100–300 line standalone spec (no shared helpers in this repo) and the bundle would more than double the existing e2e count. Each remains as its own PENDING project task and can be picked up incrementally without blocking the EM-UX umbrella. Verification - Two architect reviews: the first caught a critical bypass in an earlier `stripTagsToPlainText` implementation that did decode entities; the second confirmed the regex-based replacement closes the bypass and adds no new ones. - Suite: 226 tests, 224 passing. The 2 failures are pre-existing flakes (socket-state pollution in `meeting_created: fan-out…` and the count-based group-rollback race in `groups-crud.test.mjs`), both already filed as separate follow-up tasks and unrelated to this diff. - Pre-existing TS errors in the api-server are unchanged and not in files touched by this diff.
2026-04-30 23:19:40 +00:00
parent 26514e8ecc
commit 8b6eed1e59
2 changed files with 104 additions and 12 deletions
@@ -92,6 +92,20 @@ export function sanitizePlainTextOrNull(input: unknown): string | null {
  return s === "" ? null : s;
 }

+// Tag bodies that must be discarded along with the tag itself — script
+// contents, CSS, embedded objects, etc. would otherwise leak into the
+// stored "plain text" as readable source code.
+const DANGEROUS_TAG_BODY_RE =
+  /<(script|style|noscript|iframe|object|embed|template)\b[^>]*>[\s\S]*?<\/\1\s*>/gi;
+const HTML_COMMENT_RE = /<!--[\s\S]*?-->/g;
+const HTML_CDATA_RE = /<!\[CDATA\[[\s\S]*?\]\]>/g;
+const HTML_DOCTYPE_RE = /<!DOCTYPE[^>]*>/gi;
+const HTML_PROCINST_RE = /<\?[\s\S]*?\?>/g;
+// Matches any opening or closing HTML tag (`<tag …>`, `</tag>`). A bare
+// `<` followed by whitespace, a digit, or another `<` does NOT match —
+// so legitimate text like "5 < 10" is preserved.
+const HTML_TAG_RE = /<\/?[a-zA-Z][^>]*>/g;
+
 /**
 * Strip ALL HTML tags but PRESERVE stray special characters (`&`, `<`,
 * `>`, quotes) as their literal plain-text form. Use for free-form
@@ -99,8 +113,19 @@ export function sanitizePlainTextOrNull(input: unknown): string | null {
 * and note bodies — where entity-encoding would corrupt valid input
 * (e.g. `?a=1&b=2` → `?a=1&amp;b=2`).
 *
+ * Implementation note: this is intentionally a regex-based stripper
+ * (NOT sanitize-html + entity decode). sanitize-html unconditionally
+ * HTML-escapes its output, so the only way to reverse that is to
+ * decode entities — but a decode pass also turns attacker-supplied
+ * `&lt;script&gt;` into a literal `<script>` tag in the DB. The regex
+ * approach removes real tags without ever decoding entities, so an
+ * encoded payload survives as inert text.
+ *
 * Behavior:
- * - `<script>alert(1)</script>` → "" (tag and content discarded)
+ * - `<script>alert(1)</script>` → "" (tag AND inner JS body dropped)
+ * - `<style>...</style>` / `<iframe>...</iframe>` → "" (body dropped)
+ * - `<b>bold</b> hi` → "bold hi" (inline tag dropped, text kept)
+ * - `&lt;script&gt;…&lt;/script&gt;` → unchanged (no entity decode)
 * - `https://x.test?a=1&b=2#frag` → unchanged
 * - `5 < 10 and 20 > 5` → unchanged
 *
@@ -110,17 +135,19 @@ export function stripTagsToPlainText(input: unknown): string {
  if (input === null || input === undefined) return "";
  const s = typeof input === "string" ? input : String(input);
  if (s.length === 0) return "";
-  const stripped = sanitizeHtml(s, { allowedTags: [], allowedAttributes: {} });
-  return stripped
-    .replace(/&amp;/g, "&")
-    .replace(/&lt;/g, "<")
-    .replace(/&gt;/g, ">")
-    .replace(/&quot;/g, '"')
-    .replace(/&#39;/g, "'")
-    .replace(/&#x27;/g, "'")
-    .replace(/&#x2F;/g, "/")
-    .replace(/&#96;/g, "`")
-    .replace(/&#x60;/g, "`");
+  // Pass 1: drop tags whose body is not human-readable text along with
+  // their content, so e.g. `<script>alert(1)</script>` doesn't leak the
+  // literal string `alert(1)` into the DB.
+  let out = s.replace(DANGEROUS_TAG_BODY_RE, "");
+  // Pass 2: strip remaining HTML constructs (comments, CDATA, DOCTYPE,
+  // processing instructions, and any leftover open/close tags).
+  out = out
+    .replace(HTML_COMMENT_RE, "")
+    .replace(HTML_CDATA_RE, "")
+    .replace(HTML_DOCTYPE_RE, "")
+    .replace(HTML_PROCINST_RE, "")
+    .replace(HTML_TAG_RE, "");
+  return out;
 }

 /**
@@ -479,6 +479,71 @@ test("Sanitization: POST strips HTML from location, meetingUrl, and notes", asyn
  );
 });

+// Regression guard: an earlier sanitizer used a "strip tags then decode
+// HTML entities" approach, which let a payload like `&lt;script&gt;…`
+// round-trip into the DB as a literal `<script>` tag. The current
+// implementation (regex-based, no entity decode) must keep
+// attacker-supplied entity-encoded markup as inert text.
+test("Sanitization: encoded HTML payloads (&lt;script&gt;…) are NOT decoded into live tags", async () => {
+  const create = await api(adminCookie, "POST", "/api/executive-meetings", {
+    titleAr: "ه",
+    titleEn: "H",
+    meetingDate: today,
+    location: "&lt;script&gt;alert('loc')&lt;/script&gt;Boardroom",
+    meetingUrl:
+      "&#x3C;script&#x3E;alert('url')&#x3C;/script&#x3E;https://x.test/?a=1&b=2",
+    notes: "ok &amp; safe — &lt;img onerror=alert(1) src=x&gt;",
+    attendees: [],
+  });
+  assert.equal(create.status, 201);
+  const meeting = await create.json();
+  created.meetingIds.push(meeting.id);
+
+  // Critical: NO live tag may appear in stored values, regardless of
+  // whether the client encoded their payload.
+  for (const field of ["location", "meetingUrl", "notes"]) {
+    const v = meeting[field] ?? "";
+    assert.ok(
+      !/<script|<img|<iframe|<\/script/i.test(v),
+      `${field} must not contain live HTML tags after encoded payload, got: ${v}`,
+    );
+  }
+  // The encoded payload must survive as text, exactly as the client sent
+  // it — proving we did not decode entities.
+  assert.ok(
+    (meeting.location ?? "").includes("&lt;script&gt;"),
+    `location must keep encoded entities as text, got: ${meeting.location}`,
+  );
+  assert.ok(
+    (meeting.meetingUrl ?? "").includes("&#x3C;script&#x3E;"),
+    `meetingUrl must keep encoded entities as text, got: ${meeting.meetingUrl}`,
+  );
+  assert.ok(
+    (meeting.notes ?? "").includes("&amp;") &&
+      (meeting.notes ?? "").includes("&lt;img"),
+    `notes must keep encoded entities as text, got: ${meeting.notes}`,
+  );
+
+  // Mixed-case tag names must also be stripped (regex must be /…/i).
+  const mixed = await api(adminCookie, "POST", "/api/executive-meetings", {
+    titleAr: "خ",
+    titleEn: "X",
+    meetingDate: today,
+    location: "<ScRiPt>alert(1)</ScRiPt>Boardroom B",
+    notes: "<IFRAME src=evil>x</IFRAME>visible note",
+    attendees: [],
+  });
+  assert.equal(mixed.status, 201);
+  const mm = await mixed.json();
+  created.meetingIds.push(mm.id);
+  assert.ok(
+    !/<script|<iframe/i.test((mm.location ?? "") + (mm.notes ?? "")),
+    "mixed-case dangerous tags must be stripped (regex case-insensitive)",
+  );
+  assert.ok((mm.location ?? "").includes("Boardroom B"));
+  assert.ok((mm.notes ?? "").includes("visible note"));
+});
+
 test("Sanitization: PATCH strips HTML from location, meetingUrl, and notes", async () => {
  const create = await api(adminCookie, "POST", "/api/executive-meetings", {
    titleAr: "م",