From 3e83b568e4dbca18da9a5435ea526f9532afeac9 Mon Sep 17 00:00:00 2001 From: Thomas Berdy Date: Tue, 4 Aug 2026 19:06:46 +0200 Subject: [PATCH] fix(docs): sanitize the HTML the schema-driven tables inject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `renderMarkdown` output goes straight into `dangerouslySetInnerHTML` in six tables, and it did not filter URL protocols: `remark-rehype` emits an `` for any link target whatever its scheme, so a `javascript:` link in a synced description would have rendered as a live link. Raw HTML was never a way in — `remark-rehype` runs without `allowDangerousHtml`, so it is discarded before becoming a node — which makes URL filtering the whole of what `rehype-sanitize` adds here. It sits after `rehype-raw` so that if a caller ever does enable `allowDangerousHtml`, the embedded markup is parsed and then sanitized rather than passed through opaque. Every input today is first-party: descriptions generated by our own engine and delivered by our own sync bot. So this is defence in depth rather than a live exposure — but "the input is trustworthy" is a property of the six current callers, not of the function, and the output lands somewhere that makes the distinction expensive to get wrong later. The tests separate the two guarantees on purpose. Asserting all four cases together would have been misleading: three of them pass with the sanitizer removed, because they test the markdown pipeline discarding raw HTML rather than anything the sanitizer does. Only the URL-protocol cases fail if it goes. Output is unchanged across every description the two schemas publish. Part of MRGFY-8330 Co-Authored-By: Claude Opus 5 (1M context) Change-Id: Ice1642f855d6698b65a034e398affbce16f822a6 --- package.json | 1 + pnpm-lock.yaml | 20 ++++++++++ src/components/Tables/utils.test.ts | 59 +++++++++++++++++++++++++++++ src/components/Tables/utils.ts | 20 +++++++++- 4 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 src/components/Tables/utils.test.ts diff --git a/package.json b/package.json index a85c959df1..30a797ba8f 100644 --- a/package.json +++ b/package.json @@ -102,6 +102,7 @@ "rehype-autolink-headings": "^7.1.0", "rehype-format": "^5.0.1", "rehype-raw": "^7.0.0", + "rehype-sanitize": "^6.0.0", "rehype-slug": "^6.0.0", "rehype-stringify": "^10.0.1", "remark-lint": "^10.0.1", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c89effd9ab..55f11daf5d 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -94,6 +94,9 @@ importers: rehype-raw: specifier: ^7.0.0 version: 7.0.0 + rehype-sanitize: + specifier: ^6.0.0 + version: 6.0.0 rehype-slug: specifier: ^6.0.0 version: 6.0.0 @@ -3155,6 +3158,9 @@ packages: hast-util-raw@9.1.0: resolution: {integrity: sha512-Y8/SBAHkZGoNkpzqqfCldijcuUKh7/su31kEBp67cFY09Wy0mTRgtsLYsiIxMJxlu0f6AA5SUTbDR8K0rxnbUw==} + hast-util-sanitize@5.0.2: + resolution: {integrity: sha512-3yTWghByc50aGS7JlGhk61SPenfE/p1oaFeNwkOOyrscaOkMGrcW9+Cy/QAIOBpZxP1yqDIzFMR0+Np0i0+usg==} + hast-util-select@6.0.4: resolution: {integrity: sha512-RqGS1ZgI0MwxLaKLDxjprynNzINEkRHY2i8ln4DDjgv9ZhcYVIHN9rlpiYsqtFwrgpYU361SyWDQcGNIBVu3lw==} @@ -4336,6 +4342,9 @@ packages: rehype-recma@1.0.0: resolution: {integrity: sha512-lqA4rGUf1JmacCNWWZx0Wv1dHqMwxzsDWYMTowuplHF3xH0N/MmrZ/G3BDZnzAkRmxDadujCjaKM2hqYdCBOGw==} + rehype-sanitize@6.0.0: + resolution: {integrity: sha512-CsnhKNsyI8Tub6L4sm5ZFsme4puGfc6pYylvXo1AeqaGbjOYyzNv3qZPwvs0oMJ39eryyeOdmxwUIo94IpEhqg==} + rehype-slug@6.0.0: resolution: {integrity: sha512-lWyvf/jwu+oS5+hL5eClVd3hNdmwM1kAC0BUvEGD19pajQMIzcNUd/k9GsfQ+FfECvX+JE+e9/btsKH0EjJT6A==} @@ -8921,6 +8930,12 @@ snapshots: web-namespaces: 2.0.1 zwitch: 2.0.4 + hast-util-sanitize@5.0.2: + dependencies: + '@types/hast': 3.0.5 + '@ungap/structured-clone': 1.3.1 + unist-util-position: 5.0.0 + hast-util-select@6.0.4: dependencies: '@types/hast': 3.0.5 @@ -10458,6 +10473,11 @@ snapshots: transitivePeerDependencies: - supports-color + rehype-sanitize@6.0.0: + dependencies: + '@types/hast': 3.0.5 + hast-util-sanitize: 5.0.2 + rehype-slug@6.0.0: dependencies: '@types/hast': 3.0.5 diff --git a/src/components/Tables/utils.test.ts b/src/components/Tables/utils.test.ts new file mode 100644 index 0000000000..27059bf8c6 --- /dev/null +++ b/src/components/Tables/utils.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from 'vitest'; +import { renderMarkdown } from './utils'; + +// `renderMarkdown` output is injected with `dangerouslySetInnerHTML` by every +// schema-driven table, so what it lets through is a security property. +// +// Two different layers provide that, and it is worth keeping them apart: raw +// HTML never survives because `remark-rehype` runs without +// `allowDangerousHtml`, which is true with or without the sanitizer. Only the +// URL-protocol filtering below actually exercises `rehype-sanitize` — those +// are the assertions that fail if it is removed. +describe('renderMarkdown', () => { + it('renders the markdown the schema descriptions actually use', () => { + const html = renderMarkdown('A [real link](https://example.com) and `code`.'); + expect(html).toContain('real link'); + expect(html).toContain('code'); + }); + + it('keeps relative links, anchors and mailto', () => { + expect(renderMarkdown('[a](/merge-queue/batches)')).toContain('href="/merge-queue/batches"'); + expect(renderMarkdown('[a](#batch-status)')).toContain('href="#batch-status"'); + expect(renderMarkdown('[a](mailto:x@example.com)')).toContain('href="mailto:x@example.com"'); + }); + + // These are the sanitizer's own guarantee: `remark-rehype` emits an for + // any link target, whatever its protocol, so without `rehype-sanitize` each + // of these renders as a live link. + describe('URL protocol filtering (rehype-sanitize)', () => { + it('strips a javascript: link rather than emitting a live one', () => { + const html = renderMarkdown('[click](javascript:alert(1))'); + expect(html).toContain('click'); + expect(html).not.toContain('javascript:'); + }); + + it('strips a case-obfuscated javascript: link', () => { + expect(renderMarkdown('[click](JaVaScRiPt:alert(1))')).not.toContain('alert(1)'); + }); + + it('strips a data: URL on an image', () => { + expect(renderMarkdown('![x](data:text/html;base64,PHNjcmlwdD4=)')).not.toContain( + 'data:text/html' + ); + }); + }); + + // Kept as a regression pin on the pipeline as a whole, not on the sanitizer: + // these pass because raw HTML is discarded before it becomes a node. If a + // caller ever enables `allowDangerousHtml`, `rehype-raw` parses it and the + // sanitizer becomes what keeps these green. + describe('raw HTML never reaches the output', () => { + it('drops an event handler', () => { + expect(renderMarkdown('')).not.toContain('onerror'); + }); + + it('drops a script tag', () => { + expect(renderMarkdown('')).not.toContain('