🔒 Escaped HTML in bookmark text fields (#1394)

ref https://linear.app/ghost/issue/ENG-1751 - to prevent potential cross-site scripting (XSS) vulnerabilities or HTML injection, we are now escaping bookmark text fields for email - bookmark text fields were already escaped for web
TryGhost · Nov 13, 2024 · 9ed048c · 9ed048c
1 parent f51711f
commit 9ed048c
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 52 deletions.
diff --git a/packages/kg-default-nodes/lib/nodes/bookmark/bookmark-renderer.js b/packages/kg-default-nodes/lib/nodes/bookmark/bookmark-renderer.js
@@ -1,5 +1,7 @@
 import {addCreateDocumentOption} from '../../utils/add-create-document-option';
 import {renderEmptyContainer} from '../../utils/render-empty-container';
+import {escapeHtml} from '../../utils/escape-html';
+import {truncateHtml} from '../../utils/truncate';
 
 export function renderBookmarkNode(node, options = {}) {
     addCreateDocumentOption(options);
@@ -17,63 +19,20 @@ export function renderBookmarkNode(node, options = {}) {
     }
 }
 
-function escapeHtml(unsafe) {
-    return unsafe
-        .replace(/&/g, '&amp;')
-        .replace(/</g, '&lt;')
-        .replace(/>/g, '&gt;')
-        .replace(/"/g, '&quot;')
-        .replace(/'/g, '&#039;');
-}
-
-function truncateText(text, maxLength) {
-    if (text && text.length > maxLength) {
-        return text.substring(0, maxLength - 1).trim() + '…';
-    } else {
-        return text ?? '';
-    }
-}
-
-function truncateHtml(text, maxLength, maxLengthMobile) {
-    // If no mobile length specified or mobile length is larger than desktop,
-    // just do a simple truncate
-    if (!maxLengthMobile || maxLength <= maxLengthMobile) {
-        return escapeHtml(truncateText(text, maxLength));
-    }
-
-    // Handle text shorter than mobile length
-    if (text.length <= maxLengthMobile) {
-        return escapeHtml(text);
-    }
-
-    if (text && text.length > maxLengthMobile) {
-        let ellipsis = '';
-
-        if (text.length > maxLengthMobile && text.length <= maxLength) {
-            ellipsis = '<span class="hide-desktop">…</span>';
-        } else if (text.length > maxLength) {
-            ellipsis = '…';
-        }
-
-        return escapeHtml(text.substring(0, maxLengthMobile - 1)) + '<span class="desktop-only">' + escapeHtml(text.substring(maxLengthMobile - 1, maxLength - 1)) + '</span>' + ellipsis;
-    } else {
-        return escapeHtml(text ?? '');
-    }
-}
-
 function emailTemplate(node, document) {
-    const title = node.title;
-    const publisher = node.publisher;
-    const author = node.author;
+    const title = escapeHtml(node.title);
+    const publisher = escapeHtml(node.publisher);
+    const author = escapeHtml(node.author);
+    const description = escapeHtml(node.description);
+
     const icon = node.icon;
-    const description = node.description;
     const url = node.url;
     const thumbnail = node.thumbnail;
     const caption = node.caption;
 
     const element = document.createElement('div');
 
-    const html = 
+    const html =
         `
         <!--[if !mso !vml]-->
             <figure class="kg-card kg-bookmark-card ${caption ? `kg-card-hascaption` : ''}">

diff --git a/packages/kg-default-nodes/lib/utils/escape-html.js b/packages/kg-default-nodes/lib/utils/escape-html.js
@@ -1,6 +1,6 @@
 /**
  * Escape HTML special characters
- * @param {string} unsafe 
+ * @param {string} unsafe
  * @returns string
  */
 export function escapeHtml(unsafe) {

diff --git a/packages/kg-default-nodes/lib/utils/truncate.js b/packages/kg-default-nodes/lib/utils/truncate.js
@@ -0,0 +1,36 @@
+import {escapeHtml} from './escape-html';
+
+export function truncateText(text, maxLength) {
+    if (text && text.length > maxLength) {
+        return text.substring(0, maxLength - 1).trim() + '…';
+    } else {
+        return text ?? '';
+    }
+}
+
+export function truncateHtml(text, maxLength, maxLengthMobile) {
+    // If no mobile length specified or mobile length is larger than desktop,
+    // just do a simple truncate
+    if (!maxLengthMobile || maxLength <= maxLengthMobile) {
+        return escapeHtml(truncateText(text, maxLength));
+    }
+
+    // Handle text shorter than mobile length
+    if (text.length <= maxLengthMobile) {
+        return escapeHtml(text);
+    }
+
+    if (text && text.length > maxLengthMobile) {
+        let ellipsis = '';
+
+        if (text.length > maxLengthMobile && text.length <= maxLength) {
+            ellipsis = '<span class="hide-desktop">…</span>';
+        } else if (text.length > maxLength) {
+            ellipsis = '…';
+        }
+
+        return escapeHtml(text.substring(0, maxLengthMobile - 1)) + '<span class="desktop-only">' + escapeHtml(text.substring(maxLengthMobile - 1, maxLength - 1)) + '</span>' + ellipsis;
+    } else {
+        return escapeHtml(text ?? '');
+    }
+}
diff --git a/packages/kg-default-nodes/test/nodes/bookmark.test.js b/packages/kg-default-nodes/test/nodes/bookmark.test.js
@@ -133,7 +133,7 @@ describe('BookmarkNode', function () {
     describe('urlTransformMap', function () {
         it('contains the expected URL mapping', editorTest(function () {
             BookmarkNode.urlTransformMap.should.deepEqual({
-                'url': 'url',
+                url: 'url',
                 'metadata.icon': 'url',
                 'metadata.thumbnail': 'url'
             });
@@ -206,6 +206,64 @@ describe('BookmarkNode', function () {
 
             element.outerHTML.should.equal('<span></span>');
         }));
+
+        it('escapes HTML for text fields in web', editorTest(function () {
+            dataset = {
+                url: 'https://www.fake.org/',
+                metadata: {
+                    icon: 'https://www.fake.org/favicon.ico',
+                    title: 'Ghost: Independent technology <script>alert("XSS")</script> for modern publishing.',
+                    description: 'doing "kewl" stuff',
+                    author: 'fa\'ker',
+                    publisher: 'Fake <script>alert("XSS")</script>',
+                    thumbnail: 'https://fake.org/image.png'
+                },
+                caption: '<p dir="ltr"><span style="white-space: pre-wrap;">This is a </span><b><strong style="white-space: pre-wrap;">caption</strong></b></p>'
+            };
+            const bookmarkNode = $createBookmarkNode(dataset);
+            const {element} = bookmarkNode.exportDOM(exportOptions);
+
+            // Check that text fields are escaped
+            element.innerHTML.should.containEql('Ghost: Independent technology &lt;script&gt;alert("XSS")&lt;/script&gt; for modern publishing.');
+            element.innerHTML.should.containEql('doing "kewl" stuff');
+            element.innerHTML.should.containEql('fa\'ker');
+            element.innerHTML.should.containEql('Fake &lt;script&gt;alert("XSS")&lt;/script&gt;');
+
+            // Check that caption is not escaped
+            element.innerHTML.should.containEql('<p dir="ltr"><span style="white-space: pre-wrap;">This is a </span><b><strong style="white-space: pre-wrap;">caption</strong></b></p>');
+        }));
+
+        it('escapes HTML for text fields in email', editorTest(function () {
+            const options = {
+                target: 'email'
+            };
+            dataset = {
+                url: 'https://www.fake.org/',
+                metadata: {
+                    icon: 'https://www.fake.org/favicon.ico',
+                    title: 'Ghost: Independent technology <script>alert("XSS")</script> for modern publishing.',
+                    description: 'doing "kewl" stuff',
+                    author: 'fa\'ker',
+                    publisher: 'Fake <script>alert("XSS")</script>',
+                    thumbnail: 'https://fake.org/image.png'
+                },
+                caption: '<p dir="ltr"><span style="white-space: pre-wrap;">This is a </span><b><strong style="white-space: pre-wrap;">caption</strong></b></p>'
+            };
+            const bookmarkNode = $createBookmarkNode(dataset);
+            const {element} = bookmarkNode.exportDOM({...exportOptions, ...options});
+
+            // Check that email template is used
+            element.innerHTML.should.containEql('<!--[if !mso !vml]-->');
+
+            // Check that text fields are escaped
+            element.innerHTML.should.containEql('Ghost: Independent technology &lt;script&gt;alert("XSS")&lt;/script&gt; for modern publishing.');
+            element.innerHTML.should.containEql('doing &amp;quot;kewl&amp;quot; stuff');
+            element.innerHTML.should.containEql('fa\'ker');
+            element.innerHTML.should.containEql('Fake &lt;script&gt;alert("XSS")&lt;/script&gt;');
+
+            // Check that caption is not escaped
+            element.innerHTML.should.containEql('<p dir="ltr"><span style="white-space: pre-wrap;">This is a </span><b><strong style="white-space: pre-wrap;">caption</strong></b></p>');
+        }));
     });
 
     describe('exportJSON', function () {
@@ -277,7 +335,7 @@ describe('BookmarkNode', function () {
 
         it('urlTransformMap', editorTest(function () {
             BookmarkNode.urlTransformMap.should.deepEqual({
-                'url': 'url',
+                url: 'url',
                 'metadata.icon': 'url',
                 'metadata.thumbnail': 'url'
             });