rafraa/TX
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Refine text sanitization and update test descriptions

Browse Source 
Improve text sanitization logic and update comments in test files to be more concise and informative.
This commit is contained in:
Riyadh
2026-04-30 23:31:57 +00:00
parent 5a66000d65
commit 7dc153c10f
4 changed files with 36 additions and 178 deletions
Download Patch File Download Diff File Expand all files Collapse all files
artifacts/api-server/src/lib/sanitize.ts
+5 -38
View File
@@ -92,55 +92,26 @@ 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
* fields that are NOT interpolated into HTML — URLs, location strings,
* 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 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
*
* Empty / null / undefined input becomes "".
* Strip HTML tags but preserve stray `&`/`<`/`>`/quote characters
* literally (no entity encoding). Use for URLs and free-form text
* where entity-encoding would corrupt valid input (e.g. `?a=1&b=2`).
* Entities in the input are NOT decoded, so `&lt;script&gt;` stays
* inert. Empty/null/undefined → "".
*/
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 "";
// 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, "")
@@ -150,10 +121,6 @@ export function stripTagsToPlainText(input: unknown): string {
return out;
}
/**
* Same as {@link stripTagsToPlainText} but preserves null for nullable
* DB columns (location, meetingUrl, notes).
*/
export function stripTagsToPlainTextOrNull(input: unknown): string | null {
if (input === null || input === undefined) return null;
const s = stripTagsToPlainText(input);
artifacts/api-server/tests/executive-meetings.test.mjs
+25 -120
View File
@@ -418,23 +418,15 @@ test("Meetings: POST /duplicate clones a meeting onto another date", async () =>
assert.ok(dayBody.meetings.some((m) => m.id === newRow.id));
});
// --- Plain-text sanitization for location / meetingUrl / notes (#189) ----
// Mirrors the existing attendee-title sanitization so a meeting payload
// cannot smuggle <script> / <img onerror=...> markup into fields that
// templates (PDF renderer, audit-log popovers) interpolate as HTML.
// #189: plain-text sanitization for location / meetingUrl / notes.
test("Sanitization: POST strips HTML from location, meetingUrl, and notes", async () => {
const create = await api(adminCookie, "POST", "/api/executive-meetings", {
titleAr: "ل",
titleEn: "L",
meetingDate: today,
location: '<script>alert("xss")</script>Conference Room A',
// URL with query params + fragment + ampersand — must round-trip
// unchanged after tag removal so `meet?a=1&b=2` doesn't get stored
// as `meet?a=1&amp;b=2`.
meetingUrl:
'<img src=x onerror=alert(1)>https://example.com/meet?a=1&b=2#room',
// Notes with stray `<` (math-style) plus a real script tag — the
// script must be stripped, the `5 < 10` text must survive intact.
notes: '<b>bold</b> note: 5 < 10 & ok <script>bad()</script>',
attendees: [],
});
@@ -442,48 +434,17 @@ test("Sanitization: POST strips HTML from location, meetingUrl, and notes", asyn
const meeting = await create.json();
created.meetingIds.push(meeting.id);
assert.ok(
!/<script/i.test(meeting.location ?? ""),
`location should not contain <script>, got: ${meeting.location}`,
);
assert.ok(
(meeting.location ?? "").includes("Conference Room A"),
`location should preserve plain text, got: ${meeting.location}`,
);
assert.ok(
!/<img|onerror/i.test(meeting.meetingUrl ?? ""),
`meetingUrl should not contain <img/onerror, got: ${meeting.meetingUrl}`,
);
// Round-trip: `&` must NOT be entity-encoded.
assert.equal(
meeting.meetingUrl,
"https://example.com/meet?a=1&b=2#room",
`meetingUrl must round-trip ampersands and fragments, got: ${meeting.meetingUrl}`,
);
assert.ok(
!/<script|<b>/i.test(meeting.notes ?? ""),
`notes should strip both <b> and <script>, got: ${meeting.notes}`,
);
// Round-trip: stray `<` and `&` in plain prose must survive as literals.
assert.ok(
(meeting.notes ?? "").includes("5 < 10"),
`notes must preserve literal '<', got: ${meeting.notes}`,
);
assert.ok(
(meeting.notes ?? "").includes("& ok"),
`notes must preserve literal '&', got: ${meeting.notes}`,
);
assert.ok(
!/&amp;|&lt;|&gt;/.test(meeting.notes ?? ""),
`notes must NOT be HTML-entity-encoded, got: ${meeting.notes}`,
);
assert.ok(!/<script/i.test(meeting.location ?? ""));
assert.ok((meeting.location ?? "").includes("Conference Room A"));
assert.ok(!/<img|onerror/i.test(meeting.meetingUrl ?? ""));
assert.equal(meeting.meetingUrl, "https://example.com/meet?a=1&b=2#room");
assert.ok(!/<script|<b>/i.test(meeting.notes ?? ""));
assert.ok((meeting.notes ?? "").includes("5 < 10"));
assert.ok((meeting.notes ?? "").includes("& ok"));
assert.ok(!/&amp;|&lt;|&gt;/.test(meeting.notes ?? ""));
});
// 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.
// Regression: encoded payloads must NOT be decoded back into live tags.
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: "ه",
@@ -499,32 +460,17 @@ test("Sanitization: encoded HTML payloads (&lt;script&gt;…) are NOT decoded in
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}`,
);
assert.ok(!/<script|<img|<iframe|<\/script/i.test(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.location ?? "").includes("&lt;script&gt;"));
assert.ok((meeting.meetingUrl ?? "").includes("&#x3C;script&#x3E;"));
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",
@@ -536,10 +482,7 @@ test("Sanitization: encoded HTML payloads (&lt;script&gt;…) are NOT decoded in
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(!/<script|<iframe/i.test((mm.location ?? "") + (mm.notes ?? "")));
assert.ok((mm.location ?? "").includes("Boardroom B"));
assert.ok((mm.notes ?? "").includes("visible note"));
});
@@ -583,13 +526,9 @@ test("Sanitization: PATCH strips HTML from location, meetingUrl, and notes", asy
assert.ok((body.notes ?? "").includes("Updated note"));
});
// --- EditableCell column independence (#202) -----------------------------
// The schedule's inline title editor decides which DB column to write
// based on the active i18n locale (titleAr in ar, titleEn in en). The
// server contract that powers this must let either column be patched
// independently — patching titleEn must not clobber titleAr (and vice
// versa). This test locks down that contract so a future server-side
// regression that writes both columns from one payload fails loudly.
// #202: titleEn / titleAr are independent columns; saveTitle in
// executive-meetings.tsx routes by visible i18n.language, so a PATCH
// for one column must never clobber the other.
test("EditableCell contract: PATCH titleEn alone leaves titleAr untouched (and vice versa)", async () => {
const create = await api(adminCookie, "POST", "/api/executive-meetings", {
titleAr: "العنوان الأصلي",
@@ -601,8 +540,6 @@ test("EditableCell contract: PATCH titleEn alone leaves titleAr untouched (and v
const meeting = await create.json();
created.meetingIds.push(meeting.id);
// Mimic an `ar`-pref admin who switched UI to `en` and edited the
// English title cell — saveTitle sends only titleEn.
const patchEn = await api(
adminCookie,
"PATCH",
@@ -617,17 +554,9 @@ test("EditableCell contract: PATCH titleEn alone leaves titleAr untouched (and v
`/api/executive-meetings/${meeting.id}`,
);
let body = await fetched.json();
assert.ok(
body.titleEn.includes("Edited English"),
`titleEn should be updated, got: ${body.titleEn}`,
);
assert.ok(
body.titleAr.includes("العنوان الأصلي"),
`titleAr must NOT be clobbered by a titleEn-only PATCH, got: ${body.titleAr}`,
);
assert.ok(body.titleEn.includes("Edited English"));
assert.ok(body.titleAr.includes("العنوان الأصلي"));
// Mirror: editing the Arabic cell must not clobber the (just-edited)
// English title.
const patchAr = await api(
adminCookie,
"PATCH",
@@ -643,15 +572,9 @@ test("EditableCell contract: PATCH titleEn alone leaves titleAr untouched (and v
);
body = await fetched.json();
assert.ok(body.titleAr.includes("عنوان معدل"));
assert.ok(
body.titleEn.includes("Edited English"),
`titleEn must NOT be clobbered by a titleAr-only PATCH, got: ${body.titleEn}`,
);
assert.ok(body.titleEn.includes("Edited English"));
});
// Locks down the two indirect write paths for plain-text fields so a
// future regression in either path (forgetting the sanitizer, or
// swapping it back to one that entity-encodes) fails loudly.
test("Sanitization: duplicate path re-sanitizes location/meetingUrl/notes and preserves URL ampersands", async () => {
const create = await api(adminCookie, "POST", "/api/executive-meetings", {
titleAr: "د",
@@ -676,18 +599,10 @@ test("Sanitization: duplicate path re-sanitizes location/meetingUrl/notes and pr
const cloned = await dup.json();
created.meetingIds.push(cloned.id);
assert.equal(
cloned.meetingUrl,
"https://meet.example.com/x?room=42&token=abc",
`duplicate must round-trip URL ampersands, got: ${cloned.meetingUrl}`,
);
assert.ok(
(cloned.notes ?? "").includes("&"),
`duplicate must preserve literal '&' in notes, got: ${cloned.notes}`,
);
assert.equal(cloned.meetingUrl, "https://meet.example.com/x?room=42&token=abc");
assert.ok((cloned.notes ?? "").includes("&"));
assert.ok(
!/&amp;/.test(cloned.meetingUrl ?? "") && !/&amp;/.test(cloned.notes ?? ""),
`duplicate must not entity-encode ampersands`,
);
});
@@ -735,19 +650,9 @@ test("Sanitization: change_location approved request round-trips URL ampersands
`/api/executive-meetings/${meeting.id}`,
);
const body = await fetched.json();
assert.ok(
!/<script/i.test(body.location ?? ""),
`change_location must strip <script>, got: ${body.location}`,
);
assert.ok(
(body.location ?? "").includes("New Room (Bldg A & B)"),
`change_location must preserve literal '&' in location, got: ${body.location}`,
);
assert.equal(
body.meetingUrl,
"https://x.test/join?id=99&utm=ar",
`change_location must round-trip URL ampersands, got: ${body.meetingUrl}`,
);
assert.ok(!/<script/i.test(body.location ?? ""));
assert.ok((body.location ?? "").includes("New Room (Bldg A & B)"));
assert.equal(body.meetingUrl, "https://x.test/join?id=99&utm=ar");
});
test("Requests: POST → admin can list", async () => {