Chore: Use DOMPurify to sanitize strings rather than js-xss (#62787)

Co-authored-by: Ryan McKinley <ryantxu@gmail.com>

e2e/dashboards-suite/dashboard-templating.spec.ts

@@ -27,7 +27,7 @@ e2e.scenario({
       `Server:pipe = A'A"A|BB\\B|CCC`,
       `Server:distributed = A'A"A,Server=BB\\B,Server=CCC`,
       `Server:csv = A'A"A,BB\\B,CCC`,
-      `Server:html = A'A"A, BB\\B, CCC`,
+      `Server:html = A'A"A, BB\\B, CCC`,
       `Server:json = ["A'A\\"A","BB\\\\B","CCC"]`,
       `Server:percentencode = %7BA%27A%22A%2CBB%5CB%2CCCC%7D`,
       `Server:singlequote = 'A\\'A"A','BB\\B','CCC'`,

package.json

@@ -324,7 +324,6 @@
     "dangerously-set-html-content": "1.0.9",
     "date-fns": "2.29.3",
     "debounce-promise": "3.1.2",
-    "dompurify": "^2.4.1",
     "emotion": "11.0.0",
     "eventemitter3": "5.0.0",
     "fast-deep-equal": "^3.1.3",

packages/grafana-data/package.json

@@ -40,6 +40,7 @@
     "@types/d3-interpolate": "^3.0.0",
     "d3-interpolate": "3.0.1",
     "date-fns": "2.29.3",
+    "dompurify": "^2.4.3",
     "eventemitter3": "5.0.0",
     "fast_array_intersect": "1.1.0",
     "history": "4.10.1",
@@ -55,7 +56,7 @@
     "tinycolor2": "1.6.0",
     "tslib": "2.5.0",
     "uplot": "1.6.24",
-    "xss": "1.0.14"
+    "xss": "^1.0.14"
   },
   "devDependencies": {
     "@grafana/tsconfig": "^1.2.0-rc1",
@@ -67,6 +68,7 @@
     "@testing-library/react": "12.1.4",
     "@testing-library/react-hooks": "8.0.1",
     "@testing-library/user-event": "14.4.3",
+    "@types/dompurify": "^2",
     "@types/history": "4.7.11",
     "@types/jest": "29.2.3",
     "@types/jquery": "3.5.16",

packages/grafana-data/src/text/index.ts

@@ -1,7 +1,14 @@
 export * from './string';
 export * from './markdown';
 export * from './text';
-import { escapeHtml, hasAnsiCodes, sanitize, sanitizeUrl, sanitizeTextPanelContent } from './sanitize';
+import {
+  escapeHtml,
+  hasAnsiCodes,
+  sanitize,
+  sanitizeUrl,
+  sanitizeTextPanelContent,
+  sanitizeSVGContent,
+} from './sanitize';
 
 export const textUtil = {
   escapeHtml,
@@ -9,4 +16,5 @@ export const textUtil = {
   sanitize,
   sanitizeTextPanelContent,
   sanitizeUrl,
+  sanitizeSVGContent,
 };

packages/grafana-data/src/text/markdown.test.ts

@@ -11,6 +11,18 @@ describe('Markdown wrapper', () => {
     expect(str).toBe('<script>alert()</script>');
   });
 
+  it('should only escape (and not remove) code blocks inside markdown', () => {
+    const inlineCodeBlock = renderMarkdown('This is a piece of code block: `<script>alert()</script>`');
+    expect(inlineCodeBlock.trim()).toBe(
+      '<p>This is a piece of code block: <code>&lt;script&gt;alert()&lt;/script&gt;</code></p>'
+    );
+
+    const multilineCodeBlock = renderMarkdown('This is a piece of code block: ```<script>alert()</script>```');
+    expect(multilineCodeBlock.trim()).toBe(
+      '<p>This is a piece of code block: <code>&lt;script&gt;alert()&lt;/script&gt;</code></p>'
+    );
+  });
+
   it('should sanitize content in text panel by default', () => {
     const str = renderTextPanelMarkdown('<script>alert()</script>');
     expect(str).toBe('&lt;script&gt;alert()&lt;/script&gt;');

packages/grafana-data/src/text/markdown.ts

@@ -1,6 +1,6 @@
 import { marked } from 'marked';
 
-import { sanitize, sanitizeTextPanelContent } from './sanitize';
+import { sanitizeTextPanelContent } from './sanitize';
 
 let hasInitialized = false;
 
@@ -37,7 +37,7 @@ export function renderMarkdown(str?: string, options?: RenderMarkdownOptions): s
     return html;
   }
 
-  return sanitize(html);
+  return sanitizeTextPanelContent(html);
 }
 
 export function renderTextPanelMarkdown(str?: string, options?: RenderMarkdownOptions): string {

packages/grafana-data/src/text/sanitize.test.ts

@@ -1,4 +1,4 @@
-import { sanitizeTextPanelContent } from './sanitize';
+import { sanitizeTextPanelContent, sanitizeUrl, sanitize } from './sanitize';
 
 describe('Sanitize wrapper', () => {
   it('should allow whitelisted styles in text panel', () => {
@@ -10,3 +10,20 @@ describe('Sanitize wrapper', () => {
     );
   });
 });
+
+describe('sanitizeUrl', () => {
+  it('sanitize javascript urls', () => {
+    const url = 'javascript:alert(document.domain)';
+    const str = sanitizeUrl(url);
+    expect(str).toBe('about:blank');
+  });
+});
+
+// write test to sanitize xss payloads using the sanitize function
+describe('sanitize', () => {
+  it('should sanitize xss payload', () => {
+    const html = '<script>alert(1)</script>';
+    const str = sanitize(html);
+    expect(str).toBe('');
+  });
+});

packages/grafana-data/src/text/sanitize.ts

@@ -1,4 +1,5 @@
 import { sanitizeUrl as braintreeSanitizeUrl } from '@braintree/sanitize-url';
+import DOMPurify from 'dompurify';
 import * as xss from 'xss';
 
 const XSSWL = Object.keys(xss.whiteList).reduce<xss.IWhiteList>((acc, element) => {
@@ -6,10 +7,6 @@ const XSSWL = Object.keys(xss.whiteList).reduce<xss.IWhiteList>((acc, element) =
   return acc;
 }, {});
 
-const sanitizeXSS = new xss.FilterXSS({
-  whiteList: XSSWL,
-});
-
 const sanitizeTextPanelWhitelist = new xss.FilterXSS({
   whiteList: XSSWL,
   css: {
@@ -34,21 +31,29 @@ const sanitizeTextPanelWhitelist = new xss.FilterXSS({
 });
 
 /**
- * Returns string safe from XSS attacks.
+ * Return a sanitized string that is going to be rendered in the browser to prevent XSS attacks.
+ * Note that sanitized tags will be removed, such as "<script>".
+ * We don't allow form, pre, or input elements.
+ */
+export function sanitize(unsanitizedString: string): string {
+  try {
+    return DOMPurify.sanitize(unsanitizedString, {
+      USE_PROFILES: { html: true },
+      FORBID_TAGS: ['form', 'input', 'pre'],
+    });
+  } catch (error) {
+    console.error('String could not be sanitized', unsanitizedString);
+    return escapeHtml(unsanitizedString);
+  }
+}
+
+/**
+ * Returns string safe from XSS attacks to be used in the Text panel plugin.
  *
  * Even though we allow the style-attribute, there's still default filtering applied to it
  * Info: https://github.com/leizongmin/js-xss#customize-css-filter
  * Whitelist: https://github.com/leizongmin/js-css-filter/blob/master/lib/default.js
  */
-export function sanitize(unsanitizedString: string): string {
-  try {
-    return sanitizeXSS.process(unsanitizedString);
-  } catch (error) {
-    console.error('String could not be sanitized', unsanitizedString);
-    return unsanitizedString;
-  }
-}
-
 export function sanitizeTextPanelContent(unsanitizedString: string): string {
   try {
     return sanitizeTextPanelWhitelist.process(unsanitizedString);
@@ -58,14 +63,28 @@ export function sanitizeTextPanelContent(unsanitizedString: string): string {
   }
 }
 
+// Returns sanitized SVG, free from XSS attacks to be used when rendering SVG content.
+export function sanitizeSVGContent(unsanitizedString: string): string {
+  return DOMPurify.sanitize(unsanitizedString, { USE_PROFILES: { svg: true, svgFilters: true } });
+}
+
+// Return a sanitized URL, free from XSS attacks, such as javascript:alert(1)
 export function sanitizeUrl(url: string): string {
   return braintreeSanitizeUrl(url);
 }
 
+// Returns true if the string contains ANSI color codes.
 export function hasAnsiCodes(input: string): boolean {
   return /\u001b\[\d{1,2}m/.test(input);
 }
 
+// Returns a string with HTML entities escaped.
 export function escapeHtml(str: string): string {
-  return String(str).replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;').replace(/"/g, '&quot;');
+  return String(str)
+    .replace(/&/g, '&amp;')
+    .replace(/</g, '&lt;')
+    .replace(/>/g, '&gt;')
+    .replace(/'/g, '&#39;')
+    .replace(/\//g, '&#47;')
+    .replace(/"/g, '&quot;');
 }

public/app/core/components/SVG/SanitizedSVG.tsx

@@ -1,7 +1,8 @@
-import * as DOMPurify from 'dompurify';
 import React from 'react';
 import SVG, { Props } from 'react-inlinesvg';
 
+import { textUtil } from '@grafana/data';
+
 export const SanitizedSVG = (props: Props) => {
   return <SVG {...props} cacheRequests={true} preProcessor={getCleanSVG} />;
 };
@@ -11,7 +12,7 @@ let cache = new Map<string, string>();
 function getCleanSVG(code: string): string {
   let clean = cache.get(code);
   if (!clean) {
-    clean = DOMPurify.sanitize(code, { USE_PROFILES: { svg: true, svgFilters: true } });
+    clean = textUtil.sanitizeSVGContent(code);
     cache.set(code, clean);
   }
   return clean;

public/app/features/templating/template_srv.test.ts

@@ -336,7 +336,7 @@ describe('templateSrv', () => {
         { type: 'query', name: 'test', current: { value: '<script>alert(asd)</script>' } },
       ]);
       const target = _templateSrv.replace('$test', {}, 'html');
-      expect(target).toBe('&lt;script&gt;alert(asd)&lt;/script&gt;');
+      expect(target).toBe('&lt;script&gt;alert(asd)&lt;&#47;script&gt;');
     });
   });
 

public/app/plugins/panel/geomap/style/markers.ts

@@ -1,8 +1,7 @@
-import * as DOMPurify from 'dompurify';
 import { Fill, RegularShape, Stroke, Circle, Style, Icon, Text } from 'ol/style';
 import tinycolor from 'tinycolor2';
 
-import { Registry, RegistryItem } from '@grafana/data';
+import { Registry, RegistryItem, textUtil } from '@grafana/data';
 import { config } from '@grafana/runtime';
 import { getPublicOrAbsoluteUrl } from 'app/features/dimensions';
 
@@ -248,7 +247,7 @@ async function prepareSVG(url: string, size?: number): Promise<string> {
       return res.text();
     })
     .then((text) => {
-      text = DOMPurify.sanitize(text, { USE_PROFILES: { svg: true, svgFilters: true } });
+      text = textUtil.sanitizeSVGContent(text);
 
       const parser = new DOMParser();
       const doc = parser.parseFromString(text, 'image/svg+xml');

yarn.lock

@@ -4927,6 +4927,7 @@ __metadata:
     "@testing-library/react-hooks": 8.0.1
     "@testing-library/user-event": 14.4.3
     "@types/d3-interpolate": ^3.0.0
+    "@types/dompurify": ^2
     "@types/history": 4.7.11
     "@types/jest": 29.2.3
     "@types/jquery": 3.5.16
@@ -4941,6 +4942,7 @@ __metadata:
     "@types/tinycolor2": 1.4.3
     d3-interpolate: 3.0.1
     date-fns: 2.29.3
+    dompurify: ^2.4.3
     esbuild: 0.16.17
     eventemitter3: 5.0.0
     fast_array_intersect: 1.1.0
@@ -4967,7 +4969,7 @@ __metadata:
     tslib: 2.5.0
     typescript: 4.8.4
     uplot: 1.6.24
-    xss: 1.0.14
+    xss: ^1.0.14
   peerDependencies:
     react: ^16.8.0 || ^17.0.0
     react-dom: ^16.8.0 || ^17.0.0
@@ -18575,10 +18577,10 @@ __metadata:
   languageName: node
   linkType: hard
 
-"dompurify@npm:^2.4.1":
+"dompurify@npm:^2.4.3":
-  version: 2.4.3
+  version: 2.4.5
-  resolution: "dompurify@npm:2.4.3"
+  resolution: "dompurify@npm:2.4.5"
-  checksum: b440981f2a38cada2085759cc3d1e2f94571afc34343d011a8a6aa1ad91ae6abf651adbfa4994b0e2283f0ce81f7891cdb04b67d0b234c8d190cb70e9691f026
+  checksum: d6d3c3b320f15cdb5b26aa1902c3275a3ab2c3705a9df4420bb94691d7c4df67959ec7b91e486c308320791b0ee000456f042734c45d76721e61c2768eac706e
   languageName: node
   linkType: hard
 
@@ -22254,7 +22256,6 @@ __metadata:
     dangerously-set-html-content: 1.0.9
     date-fns: 2.29.3
     debounce-promise: 3.1.2
-    dompurify: ^2.4.1
     emotion: 11.0.0
     esbuild: 0.16.17
     esbuild-loader: 2.21.0
@@ -40045,7 +40046,7 @@ __metadata:
   languageName: node
   linkType: hard
 
-"xss@npm:1.0.14":
+"xss@npm:^1.0.14":
   version: 1.0.14
   resolution: "xss@npm:1.0.14"
   dependencies: