From cc743f5e2e1cea93ce54393a02ab53087164a63d Mon Sep 17 00:00:00 2001
From: Max Strasinsky <98811342+mstrasinskis@users.noreply.github.com>
Date: Thu, 5 Oct 2023 17:08:59 +0200
Subject: [PATCH] Escape raw html tag in markdown by default (#3399)

# Motivation

Escape raw html tags in markdown content to increase the security.
> Disadvantage is that the SVGs inside the markdown code block will be
also escaped and rendered with `&lt;` instead of `<`.

# Changes

- add optional `escapeRawHtmlTags` to `markdownToHTML` function

# Tests

- htmlRenderer

# Todos

- [x] Add entry to changelog (if necessary).

# Screenshots

There is a raw html in the [provided
sample](https://dashboard.internetcomputer.org/proposal/104084):

## before
<img width="825" alt="Screenshot 2023-09-27 at 15 20 21"
src="https://github.com/dfinity/nns-dapp/assets/98811342/6e925de2-904a-42ca-a8ba-105a181dae9a">

## after

| NNS-DAPP | Dashboard |
|--------|--------|
| <img width="544" alt="image"
src="https://github.com/dfinity/nns-dapp/assets/98811342/3a92ef4d-ff71-4c69-8be9-e689bb8853bf">
| <img width="694" alt="image"
src="https://github.com/dfinity/nns-dapp/assets/98811342/fd02cd9b-ebe2-4d43-b420-c8344814e5b8">
|
---
 CHANGELOG-Nns-Dapp.md                         |  1 +
 frontend/src/lib/utils/html.utils.ts          | 52 +++++++++++++++++--
 .../src/tests/lib/utils/html.utils.spec.ts    | 34 ++++++++++++
 3 files changed, 82 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG-Nns-Dapp.md b/CHANGELOG-Nns-Dapp.md
index 66b2a74a2da..1cf248d2bc9 100644
--- a/CHANGELOG-Nns-Dapp.md
+++ b/CHANGELOG-Nns-Dapp.md
@@ -27,6 +27,7 @@ The NNS Dapp is released through proposals in the Network Nervous System. Theref
 * Review the chunking strategy to enhance the dapp's loading time and prevent random, rare flashes of unstyled content (FOUC).
 * New header UI in the canister detail page.
 * New labels for min and max participation.
+* Improve security by escaping additional images in the proposal summary markdown.
 * Internal change: remove unused snsQueryStore.
 
 #### Deprecated
diff --git a/frontend/src/lib/utils/html.utils.ts b/frontend/src/lib/utils/html.utils.ts
index c031b498b31..7cea06b9734 100644
--- a/frontend/src/lib/utils/html.utils.ts
+++ b/frontend/src/lib/utils/html.utils.ts
@@ -1,3 +1,4 @@
+import { isNullish } from "@dfinity/utils";
 import type { marked as markedTypes, Renderer } from "marked";
 
 type Marked = typeof markedTypes;
@@ -42,24 +43,65 @@ export const imageToLinkRenderer = (
   }${titleProp ?? ""}>${text}</a>`;
 };
 
-export const renderer = (marked: Marked): Renderer => {
+const escapeHtml = (html: string): string =>
+  html.replace(/</g, "&lt;").replace(/>/g, "&gt;");
+const escapeSvgs = (html: string): string =>
+  html.replace(/<svg[^>]*>[\s\S]*?<\/svg>/gi, escapeHtml);
+
+/**
+ * Escape <img> tags or convert them to links
+ */
+const transformImg = (img: string): string => {
+  const src = img.match(/src="([^"]+)"/)?.[1];
+  const alt = img.match(/alt="([^"]+)"/)?.[1] || "img";
+  const title = img.match(/title="([^"]+)"/)?.[1];
+  const shouldEscape = isNullish(src) || src.startsWith("data:image");
+  const imageHtml = shouldEscape
+    ? escapeHtml(img)
+    : imageToLinkRenderer(src, title, alt);
+
+  return imageHtml;
+};
+
+/** Avoid <img> tags; instead, apply the same logic as for markdown images by either escaping them or converting them to links. */
+export const htmlRenderer = (html: string): string =>
+  /<img\s+[^>]*>/gi.test(html) ? transformImg(html) : html;
+
+/**
+ * Marked.js renderer for proposal summary.
+ * Customized renderers
+ * - targetBlankLinkRenderer
+ * - imageToLinkRenderer
+ * - htmlRenderer
+ *
+ * @param marked
+ */
+const proposalSummaryRenderer = (marked: Marked): Renderer => {
   const renderer = new marked.Renderer();
-  // custom link renderer
+
   renderer.link = targetBlankLinkRenderer;
   renderer.image = imageToLinkRenderer;
+  renderer.html = htmlRenderer;
 
   return renderer;
 };
 
 /**
- * Uses markedjs
+ * Uses markedjs.
+ * Escape or transform to links some raw HTML tags (img, svg)
  * @see {@link https://github.com/markedjs/marked}
  */
 export const markdownToHTML = async (text: string): Promise<string> => {
   const url = "/assets/libs/marked.min.js";
+
+  // Replace the SVG elements in the HTML with their escaped versions to improve security.
+  // It's not possible to do it with html renderer because the svg consists of multiple tags.
+  // One edge case is not covered: if the svg is inside the <code> tag, it will be rendered as with &lt; & &gt; instead of "<" & ">"
+  const escapedText = escapeSvgs(text);
+
   // The dynamic import cannot be analyzed by Vite. As it is intended, we use the /* @vite-ignore */ comment inside the import() call to suppress this warning.
   const { marked }: { marked: Marked } = await import(/* @vite-ignore */ url);
-  return marked(text, {
-    renderer: renderer(marked),
+  return marked(escapedText, {
+    renderer: proposalSummaryRenderer(marked),
   });
 };
diff --git a/frontend/src/tests/lib/utils/html.utils.spec.ts b/frontend/src/tests/lib/utils/html.utils.spec.ts
index 055c1e72d29..a04c2be02a2 100644
--- a/frontend/src/tests/lib/utils/html.utils.spec.ts
+++ b/frontend/src/tests/lib/utils/html.utils.spec.ts
@@ -1,4 +1,5 @@
 import {
+  htmlRenderer,
   imageToLinkRenderer,
   markdownToHTML,
   targetBlankLinkRenderer,
@@ -92,6 +93,32 @@ describe("markdown.utils", () => {
     });
   });
 
+  describe("htmlRenderer", () => {
+    it("should apply imageToLinkRenderer to img tag", () => {
+      const src = "image.png";
+      const title = "title";
+      const alt = "alt";
+      const expectation = imageToLinkRenderer(src, title, alt);
+      expect(
+        htmlRenderer(`<img src="${src}" alt="${alt}" title="${title}" />`)
+      ).toEqual(expectation);
+      expect(
+        htmlRenderer(`<img src="${src}" alt="${alt}" title="${title}">...`)
+      ).toEqual(expectation);
+      expect(
+        htmlRenderer(
+          `<img data-test="123" src="${src}" alt="${alt}" title="${title}" />`
+        )
+      ).toEqual(expectation);
+    });
+
+    it("should escape img tag with data src", () => {
+      expect(htmlRenderer(`<img src=""data:image/...">`)).toEqual(
+        `&lt;img src=""data:image/..."&gt;`
+      );
+    });
+  });
+
   describe("markdown", () => {
     let renderer: unknown;
 
@@ -125,8 +152,15 @@ describe("markdown.utils", () => {
         renderer: {
           link: targetBlankLinkRenderer,
           image: imageToLinkRenderer,
+          html: htmlRenderer,
         },
       });
     });
+
+    it("should escape all SVGs", async () => {
+      expect(await markdownToHTML("<h1><svg>...</svg></h1>")).toBe(
+        "<h1>&lt;svg&gt;...&lt;/svg&gt;</h1>-markdown"
+      );
+    });
   });
 });