fix: 🐛 prevent html injection via data.uri link rendering

Using a `hyperlink` node with e.g. `data.uri = '"><h1>html</h1><a href="'` it was possible to inject arbitrary HTML next to the actual hyperlink. We encode `"` within the `href` attribute. The same technique should be applied for all attributes if added in the future.
contentful · Jul 30, 2021 · ecae89a · ecae89a
1 parent a692e8e
commit ecae89a
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 3 deletions.
diff --git a/packages/rich-text-html-renderer/package-lock.json b/packages/rich-text-html-renderer/package-lock.json
diff --git a/packages/rich-text-html-renderer/package.json b/packages/rich-text-html-renderer/package.json
@@ -29,7 +29,9 @@
   },
   "devDependencies": {
     "@types/escape-html": "0.0.20",
+    "@types/lodash.clonedeep": "^4.5.6",
     "jest": "^24.7.1",
+    "lodash.clonedeep": "^4.5.0",
     "rimraf": "^2.6.3",
     "rollup": "^1.11.0",
     "rollup-plugin-commonjs": "^9.3.4",

diff --git a/packages/rich-text-html-renderer/src/__test__/index.test.ts b/packages/rich-text-html-renderer/src/__test__/index.test.ts
@@ -1,5 +1,5 @@
-import { Document, BLOCKS, MARKS, INLINES } from '@contentful/rich-text-types';
-
+import cloneDeep from 'lodash/cloneDeep';
+import { Document, Block, BLOCKS, MARKS, INLINES } from '@contentful/rich-text-types';
 import { documentToHtmlString, Options } from '../index';
 import {
   hrDoc,
@@ -227,6 +227,23 @@ describe('documentToHtmlString', () => {
     expect(documentToHtmlString(document)).toEqual(expected);
   });
 
+  it('renders hyperlink without allowing html injection via `data.uri`', () => {
+    const document: Document = cloneDeep(hyperlinkDoc);
+    (document.content[0].content[1] as Block).data.uri = '">no html injection!<a href="';
+    const expected =
+      '<p>Some text <a href="&quot;>no html injection!<a href=&quot;">link</a> text.</p>';
+
+    expect(documentToHtmlString(document)).toEqual(expected);
+  });
+
+  it('renders hyperlink without invalid non-string `data.uri` values', () => {
+    const document: Document = cloneDeep(hyperlinkDoc);
+    (document.content[0].content[1] as Block).data.uri = 42;
+    const expected = '<p>Some text <a href="">link</a> text.</p>';
+
+    expect(documentToHtmlString(document)).toEqual(expected);
+  });
+
   it(`renders asset hyperlink`, () => {
     const asset = {
       target: {

diff --git a/packages/rich-text-html-renderer/src/index.ts b/packages/rich-text-html-renderer/src/index.ts
@@ -12,6 +12,8 @@ import {
 } from '@contentful/rich-text-types';
 import React from 'react';
 
+const attributeValue = (value: string) => `"${value.replace(/"/g, '&quot;')}"`;
+
 const defaultNodeRenderers: RenderNode = {
   [BLOCKS.PARAGRAPH]: (node, next) => `<p>${next(node.content)}</p>`,
   [BLOCKS.HEADING_1]: (node, next) => `<h1>${next(node.content)}</h1>`,
@@ -32,7 +34,10 @@ const defaultNodeRenderers: RenderNode = {
   [INLINES.ASSET_HYPERLINK]: node => defaultInline(INLINES.ASSET_HYPERLINK, node as Inline),
   [INLINES.ENTRY_HYPERLINK]: node => defaultInline(INLINES.ENTRY_HYPERLINK, node as Inline),
   [INLINES.EMBEDDED_ENTRY]: node => defaultInline(INLINES.EMBEDDED_ENTRY, node as Inline),
-  [INLINES.HYPERLINK]: (node, next) => `<a href="${node.data.uri}">${next(node.content)}</a>`,
+  [INLINES.HYPERLINK]: (node, next) => {
+    const href = typeof node.data.uri === 'string' ? node.data.uri : '';
+    return `<a href=${attributeValue(href)}>${next(node.content)}</a>`;
+  },
 };
 
 const defaultMarkRenderers: RenderMark = {