diff --git a/.changeset/rich-text-display-sanitizer.md b/.changeset/rich-text-display-sanitizer.md new file mode 100644 index 0000000000..2cd49abac8 --- /dev/null +++ b/.changeset/rich-text-display-sanitizer.md @@ -0,0 +1,7 @@ +--- +'@object-ui/fields': patch +--- + +`html` and `richtext` values are now sanitized with `rehype-sanitize`, the sanitizer the `markdown` pipeline already used, instead of a hand-written pattern filter. + +A stored value is parsed with the browser's HTML parser, filtered against one allow-list of elements, attributes and URL protocols, and rendered as React elements. Both display pipelines share that allow-list. Headings, paragraphs, emphasis, lists, quotes, code, tables, links and images keep their formatting. Event-handler attributes, `style`, script-bearing URLs, embedded frames and objects, and SVG and MathML content are dropped. A link whose URL is refused now renders without an `href`, where it used to point at `#`. Stored `id` and `name` values are prefixed with `user-content-`. diff --git a/content/docs/fields/rich-text.mdx b/content/docs/fields/rich-text.mdx index 7dcb83e48c..95a90f7a20 100644 --- a/content/docs/fields/rich-text.mdx +++ b/content/docs/fields/rich-text.mdx @@ -94,10 +94,14 @@ metadata keys — they are runtime widget props. See [Field Widget Props](/docs/ - **Tables**: Markdown tables ### HTML -- Full HTML editing support -- Sanitization for security -- Style preservation -- Embedded media support +- Headings, paragraphs, emphasis, lists, quotes, code, tables, links and images +- Sanitized on display against an allow-list (`rehype-sanitize`, GitHub-style): + links keep `http`, `https`, `mailto` and relative URLs, and images keep `http`, + `https` and relative URLs +- Inline `style`, event-handler attributes, embedded frames, objects and SVG or + MathML content are not rendered; an element outside the allow-list shows its text + without its tag +- The `markdown` and `html` / `richtext` display pipelines share the same allow-list ## Cell Renderer diff --git a/packages/fields/package.json b/packages/fields/package.json index 6102134c98..ebb960550f 100644 --- a/packages/fields/package.json +++ b/packages/fields/package.json @@ -38,6 +38,8 @@ "@object-ui/react": "workspace:*", "@object-ui/types": "workspace:*", "@objectstack/spec": "^18.0.0-next.1", + "hast-util-to-jsx-runtime": "^2.3.6", + "hastscript": "^9.0.1", "lucide-react": "^1.43.0", "react-markdown": "^10.1.0", "rehype-sanitize": "^6.0.0", diff --git a/packages/fields/src/__tests__/cellRenderers.childlessContainer-8580.test.tsx b/packages/fields/src/__tests__/cellRenderers.childlessContainer-8580.test.tsx index d27550a519..bed04d66b2 100644 --- a/packages/fields/src/__tests__/cellRenderers.childlessContainer-8580.test.tsx +++ b/packages/fields/src/__tests__/cellRenderers.childlessContainer-8580.test.tsx @@ -51,7 +51,7 @@ * case in this file. What refuses it is the POPULATED block: real markdown * still renders its elements, real HTML still renders SANITISED — pinned to * the sanitiser's exact output bytes, so a repair that shortcut the populated - * branch past `sanitizeHtml` fails on bytes, not on a count — and real + * branch past the sanitizer fails on bytes, not on a count — and real * richtext still renders. Each was RUN against the caricature; the observed * failure sentences are recorded in the PR. * @@ -127,8 +127,8 @@ const HOSTILE_HTML = '' + 'go'; -/** What `sanitizeHtml` makes of {@link HOSTILE_HTML} — the bytes, not a count. */ -const HOSTILE_HTML_SANITISED = '

ok

go'; +/** What the sanitizer makes of {@link HOSTILE_HTML} — the bytes, not a count. */ +const HOSTILE_HTML_SANITISED = '

ok

go'; /** What a WYSIWYG editor actually emits, beyond the seed's two tags. */ const WYSIWYG_DOCUMENT = [ @@ -241,10 +241,10 @@ describe('objectui#8580 — a rich-content cell holding [] or {} says what it ho const prose = container.querySelector('div.prose'); expect(prose, 'html: a populated value must still open its prose container').not.toBeNull(); - // The bytes, not a count: this is what `sanitizeHtml` produces for the + // The bytes, not a count: this is what the sanitizer produces for the // hostile document, so a populated branch that stopped running through // it — or a sanitiser that changed — fails here on content. - expect(prose?.innerHTML, 'html: the populated branch must still run through sanitizeHtml').toBe(HOSTILE_HTML_SANITISED); + expect(prose?.innerHTML, 'html: the populated branch must still run through the sanitizer').toBe(HOSTILE_HTML_SANITISED); expect(prose?.querySelector('script'), 'html: script blocks must still be stripped').toBeNull(); expect(affordance(container), 'html: a populated value must NOT render the affordance').toBeNull(); }); @@ -254,7 +254,7 @@ describe('objectui#8580 — a rich-content cell holding [] or {} says what it ho const prose = container.querySelector('div.prose'); expect(prose, 'richtext: a populated value must still open its prose container').not.toBeNull(); - expect(prose?.innerHTML, 'richtext: the populated branch must still run through sanitizeHtml').toBe(HOSTILE_HTML_SANITISED); + expect(prose?.innerHTML, 'richtext: the populated branch must still run through the sanitizer').toBe(HOSTILE_HTML_SANITISED); expect(affordance(container), 'richtext: a populated value must NOT render the affordance').toBeNull(); }); diff --git a/packages/fields/src/__tests__/readonly-richtext-display-5498.test.tsx b/packages/fields/src/__tests__/readonly-richtext-display-5498.test.tsx index 5b3501c42f..fb8b52cdc8 100644 --- a/packages/fields/src/__tests__/readonly-richtext-display-5498.test.tsx +++ b/packages/fields/src/__tests__/readonly-richtext-display-5498.test.tsx @@ -230,7 +230,8 @@ describe('negative controls — nothing else became a markup renderer (objectui# expect(prose.querySelector('script')).toBeNull(); expect(prose.innerHTML).not.toContain('alert(1)'); expect(prose.innerHTML).not.toContain('onerror'); - expect(prose.querySelector('a')?.getAttribute('href')).not.toContain('javascript:'); + // A dropped `href` reads `null`; a rewritten one must not name the scheme. + expect(prose.querySelector('a')?.getAttribute('href') ?? '').not.toContain('javascript:'); // …and the benign content around it survived, so the assertion above is // not passing because the whole value was dropped. expect(prose.textContent).toContain('ok'); diff --git a/packages/fields/src/__tests__/richtext-cell-renderer-5452.test.tsx b/packages/fields/src/__tests__/richtext-cell-renderer-5452.test.tsx index ac7b9a4662..4ce942bd7a 100644 --- a/packages/fields/src/__tests__/richtext-cell-renderer-5452.test.tsx +++ b/packages/fields/src/__tests__/richtext-cell-renderer-5452.test.tsx @@ -125,7 +125,8 @@ describe('rich-content cell renderers (objectui#5452)', () => { expect(prose.querySelector('script')).toBeNull(); expect(prose.innerHTML).not.toContain('onerror'); - expect(prose.querySelector('a')?.getAttribute('href')).not.toContain('javascript:'); + // A dropped `href` reads `null`; a rewritten one must not name the scheme. + expect(prose.querySelector('a')?.getAttribute('href') ?? '').not.toContain('javascript:'); expect(prose.textContent).toContain('ok'); }); diff --git a/packages/fields/src/widgets/MarkdownContent.tsx b/packages/fields/src/widgets/MarkdownContent.tsx index 5ca6ac1639..b07eaf2f67 100644 --- a/packages/fields/src/widgets/MarkdownContent.tsx +++ b/packages/fields/src/widgets/MarkdownContent.tsx @@ -2,16 +2,18 @@ import React from 'react'; import ReactMarkdown from 'react-markdown'; import remarkGfm from 'remark-gfm'; import rehypeSanitize from 'rehype-sanitize'; +import { RICH_TEXT_SANITIZE_SCHEMA } from './sanitizedHtml.js'; /** * Heavy markdown renderer, split into its own module so it can be lazy-loaded * by the cell/detail renderers (react-markdown pulls in ~100-200 KB). GitHub - * Flavored Markdown + sanitization (defense against XSS in stored content). + * Flavored Markdown + sanitization (defense against XSS in stored content), + * against the same schema the `html` / `richtext` pipeline uses. */ export default function MarkdownContent({ value }: { value: string }) { return (
- + {value}
diff --git a/packages/fields/src/widgets/RichTextField.tsx b/packages/fields/src/widgets/RichTextField.tsx index 1167247d39..597d3cb8ee 100644 --- a/packages/fields/src/widgets/RichTextField.tsx +++ b/packages/fields/src/widgets/RichTextField.tsx @@ -261,7 +261,7 @@ function resolveRichTextFieldType(field: unknown): string { * It now renders through {@link richTextCellRenderer} — literally the same * components `getCellRenderer` resolves, one table shared by both (see * `./richTextDisplay.tsx`), so the html/richtext path carries that renderer's - * `sanitizeHtml` trust boundary rather than a second hand-rolled escape, and a + * sanitizing trust boundary rather than a second hand-rolled escape, and a * future change to either pipeline moves both surfaces at once. * * ## Host plumbing (objectui#4810) diff --git a/packages/fields/src/widgets/__tests__/sanitizedHtml.test.tsx b/packages/fields/src/widgets/__tests__/sanitizedHtml.test.tsx new file mode 100644 index 0000000000..72abbf19ad --- /dev/null +++ b/packages/fields/src/widgets/__tests__/sanitizedHtml.test.tsx @@ -0,0 +1,204 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * The `html` / `richtext` display pipeline's trust boundary + * (`../sanitizedHtml.tsx`), read through the renderer every read surface uses. + * + * Two halves, and both are load-bearing: + * + * - HOSTILE: each class of stored markup that can carry script comes out + * inert. The assertions are STRUCTURAL — no `on*` attribute anywhere, no + * script-bearing URL in any attribute, none of the embedding / foreign + * content elements, no `style` — read off the rendered DOM, so they hold + * for any spelling of a payload within its class, not just the one + * written here. + * - FORMATTING: a representative rich-text document renders back byte for + * byte. A sanitizer that drops everything passes the first half; this + * half is what refuses it. + */ + +import React from 'react'; +import { describe, it, expect, afterEach } from 'vitest'; +import { render, cleanup } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { HtmlCellRenderer, RICH_TEXT_CELL_RENDERERS } from '../richTextDisplay'; +import { RICH_TEXT_SANITIZE_SCHEMA } from '../sanitizedHtml'; + +afterEach(cleanup); + +function renderHtml(value: string): HTMLElement { + const { container } = render( + , + ); + const prose = container.querySelector('div.prose'); + expect(prose, 'a populated value opens its prose container').not.toBeNull(); + return prose!; +} + +/** Elements whose presence alone means the value was not neutralised. */ +const FORBIDDEN_ELEMENTS = + 'script, style, iframe, frame, object, embed, applet, svg, math, link, meta, base, form, template, noscript'; + +/** Attributes that take a URL, in either namespace. */ +const URL_ATTRIBUTES = ['href', 'src', 'action', 'formaction', 'data', 'xlink:href', 'srcset', 'cite', 'poster', 'background']; + +/** Everything a hostile value must NOT have left in the DOM. */ +function expectInert(prose: HTMLElement, label: string) { + expect(prose.querySelector(FORBIDDEN_ELEMENTS), `${label}: no embedding or foreign-content element`).toBeNull(); + for (const element of Array.from(prose.querySelectorAll('*'))) { + for (const attribute of Array.from(element.attributes)) { + const name = attribute.name.toLowerCase(); + expect(name.startsWith('on'), `${label}: <${element.localName}> keeps no ${name}`).toBe(false); + expect(name, `${label}: <${element.localName}> keeps no style`).not.toBe('style'); + expect(name, `${label}: <${element.localName}> keeps no srcdoc`).not.toBe('srcdoc'); + if (URL_ATTRIBUTES.includes(name)) { + // Normalised the way a URL parser reads it: control characters and + // whitespace removed, case folded. + const url = Array.from(attribute.value) + .filter((char) => char.charCodeAt(0) > 0x20) + .join('') + .toLowerCase(); + expect(url, `${label}: <${element.localName} ${name}> is not a script URL`).not.toMatch( + /^(javascript|vbscript|data):/, + ); + } + } + } +} + +const HOSTILE: ReadonlyArray = [ + // Slash-separated attributes: no whitespace before the handler name. + ['slash-separated img handler', '

a

'], + ['slash-separated svg handler', ''], + ['slash before a quoted handler', ''], + ['handler on an allowed element', '

text

'], + ['mixed-case handler', 'x'], + // Unquoted and otherwise disguised script URLs. + ['unquoted javascript href', 'x'], + ['mixed-case javascript href', 'x'], + ['leading-space javascript href', 'x'], + ['tab inside the scheme', 'x'], + ['vbscript href', 'x'], + ['data URL href', 'x'], + ['javascript img src', ''], + // Entity-encoded schemes. + ['decimal-entity scheme', 'x'], + ['hex-entity scheme', 'x'], + ['entity-encoded tab in the scheme', 'x'], + ['padded decimal entities', 'x'], + // SVG and MathML. + ['svg animate handler', ''], + ['svg link', 'x'], + ['svg set href', 'x'], + ['svg foreignObject', ''], + ['math link', 'x'], + ['math href', 'x'], + ['math style breakout', '

x

'], + ['style attribute with an expression', '

x

'], + // Script and its neighbours. + ['script block', '

ok

'], + ['script with a src', ''], + ['nested script tags', 'ipt>alert(1)ipt>'], + ['noscript breakout', '
', + '
StageAmount
Won1,000
Total
', + '

Logo

', + '
', + '
Term
Definition
', + '
More

Hidden detail.

', +].join(''); + +describe('html / richtext display: a formatted document keeps its formatting', () => { + it('round-trips byte for byte', () => { + expect(renderHtml(FORMATTED_DOCUMENT).innerHTML).toBe(FORMATTED_DOCUMENT); + }); + + it('keeps the safe part of a mixed value', () => { + const prose = renderHtml( + '

Hello world

badgood', + ); + expect(prose.innerHTML).toBe('

Hello world

badgood'); + }); + + it('keeps the text of an element it does not allow', () => { + expect(renderHtml('

underlined and coloured

').innerHTML).toBe( + '

underlined and coloured

', + ); + }); + + it('renders plain text that holds no markup as text', () => { + expect(renderHtml('5 < 6 & 7 > 3').textContent).toBe('5 < 6 & 7 > 3'); + }); +}); diff --git a/packages/fields/src/widgets/richTextDisplay.tsx b/packages/fields/src/widgets/richTextDisplay.tsx index 422a154354..4fe7b400bc 100644 --- a/packages/fields/src/widgets/richTextDisplay.tsx +++ b/packages/fields/src/widgets/richTextDisplay.tsx @@ -34,6 +34,7 @@ import React from 'react'; import { EmptyValue } from '@object-ui/components'; import { coerceToSafeValue } from '../coerceToSafeValue.js'; +import { renderSanitizedHtml } from './sanitizedHtml.js'; import type { CellRendererProps } from '../index.js'; const LazyMarkdownContent = React.lazy(() => import('./MarkdownContent.js')); @@ -100,23 +101,6 @@ export function MarkdownCellRenderer({ value }: CellRendererProps): React.ReactE ); } -/** - * Minimal HTML sanitizer for display: drops