Fix stored HTML injection in comment markdown (GO-39)

.github/workflows/ci.yml

@@ -85,6 +85,9 @@ jobs:
       run: |
         npm install
 
+    - name: Test JavaScript helpers
+      run: npm run test:js
+
     - name: Test Syntax
       run: ./tests/test_for_syntax_errors.sh
 

dt-assets/js/activity-log.js

@@ -119,12 +119,14 @@ jQuery(function () {
       let activityDetails = '';
       let dateRegex = /\{(\d+)\}+/;
       object_notes.forEach((objectNote) => {
-        let newObjectNote = `${window.SHAREDFUNCTIONS.escapeHTML(objectNote)}`;
+        let newObjectNote = objectNote;
         if (dateRegex.test(objectNote)) {
-          newObjectNote = objectNote.replace(dateRegex, timestamptoDate);
+          newObjectNote = newObjectNote.replace(dateRegex, timestamptoDate);
         }
         if (action === 'comment') {
           newObjectNote = window.SHAREDFUNCTIONS.formatComment(newObjectNote);
+        } else {
+          newObjectNote = window.SHAREDFUNCTIONS.escapeHTML(newObjectNote);
         }
         activityDetails += `<li>${newObjectNote}</li>`;
       });

dt-assets/js/comments.js

@@ -408,23 +408,21 @@ jQuery(document).ready(function ($) {
     let array = [];
 
     displayed.forEach((d) => {
+      let objectNote = d.object_note;
       let baptismDateRegex = /\{(\d+)\}+/;
-      if (baptismDateRegex.test(d.object_note)) {
+      if (baptismDateRegex.test(objectNote)) {
         if (d.field_type === 'datetime') {
-          d.object_note = d.object_note.replace(
+          objectNote = objectNote.replace(
             baptismDateRegex,
             formatTimestampToDateTime,
           );
         } else {
-          d.object_note = d.object_note.replace(
+          objectNote = objectNote.replace(
             baptismDateRegex,
             formatTimestampToDate,
           );
         }
       }
-      if (d.object_note) {
-        d.object_note = formatComment(d.object_note);
-      }
       let first = window.lodash.first(array);
       let name = d.comment_author || d.name;
       let gravatar = d.gravatar || '';
@@ -436,7 +434,7 @@ jQuery(document).ready(function ($) {
           true,
         ),
         gravatar,
-        text: d.object_note || formatComment(d.comment_content),
+        text: formatComment(objectNote || d.comment_content),
         comment: !!d.comment_content,
         comment_ID: d.comment_ID,
         is_own_comment: d.user_id === commentsSettings.current_user_id,
@@ -611,17 +609,6 @@ jQuery(document).ready(function ($) {
         .moment(comment.comment_date_gmt + 'Z')
         .add(5, 'seconds');
 
-      /* comment_content should be HTML. However, we want to make sure that
-       * HTML like "<div>Hello" gets transformed to "<div>Hello</div>", that
-       * is, that all tags are closed, so that the comment_content can be
-       * included in HTML without any nasty surprises. This is one way to do
-       * that. This is not sufficient for malicious input, but hopefully we
-       * can trust the contents of the database to have been sanitized
-       * thanks to wp_new_comment . */
-
-      // .DT lets strip out the tags provided from the submited comment and treat it as pure text.
-      comment.comment_content = $('<div>').text(comment.comment_content).text();
-
       if (!typesCount[comment.comment_type]) {
         typesCount[comment.comment_type] = 0;
       }

dt-assets/js/shared-functions.js

@@ -769,19 +769,76 @@ window.SHAREDFUNCTIONS = {
    */
   formatComment(comment) {
     if (comment) {
-      //![image](imageUrl) becomes <img alt="image" src="imageUrl"> */
+      // WordPress comments already contain HTML entities. Preserve one level
+      // of entity encoding: references stay inert in text and quoted attributes
+      // and must not become visible as e.g. "&amp;" after rendering.
+      const escapeHTML = (value) =>
+        window.SHAREDFUNCTIONS.escapeHTML(value).replace(
+          /&amp;((?:#\d+|#x[\da-f]+|[a-z][a-z\d]+);)/gi,
+          '&$1',
+        );
+      const formatEmphasis = (text) =>
+        text
+          .replace(/\*\*(.*?)\*\*/g, '<strong>$1</strong>')
+          .replace(/__(.*?)__/g, '<strong>$1</strong>')
+          .replace(/\*(.*?)\*/g, '<em>$1</em>');
+      const isHttpUrl = (url) => {
+        try {
+          return (
+            /^https?:\/\//i.test(url) &&
+            ['http:', 'https:'].includes(new URL(url).protocol)
+          );
+        } catch (error) {
+          return false;
+        }
+      };
+
+      // Keep generated HTML out of subsequent markdown replacements. Remove
+      // input NULs so a comment cannot impersonate one of these placeholders.
+      const html = [];
+      const keepHtml = (value) => `\u0000${html.push(value) - 1}\u0000`;
+      comment = comment.split('\u0000').join('');
+
+      // Parse image, mention, and link markdown together so generated markup
+      // can never become part of another markdown capture group.
       comment = comment.replace(
-        /!\[(.*?)\]\((.*?)\)/g,
-        "<img src='$2' alt='$1'>",
+        /([!@]?)\[(.*?)\]\((.*?)\)/g,
+        (match, type, text, url) => {
+          if (type === '!') {
+            return keepHtml(
+              isHttpUrl(url)
+                ? `<img src="${escapeHTML(url)}" alt="${escapeHTML(text)}">`
+                : escapeHTML(match),
+            );
+          }
+          if (!url) {
+            return keepHtml(escapeHTML(match));
+          }
+          if (type === '@') {
+            // Keep the @ beside the name for both LTR and RTL mentions.
+            return keepHtml(
+              `<a dir="auto">@${formatEmphasis(escapeHTML(text))}</a>`,
+            );
+          }
+          if (isHttpUrl(text) && !isHttpUrl(url)) {
+            [url, text] = [text, url];
+          }
+          if (!isHttpUrl(url)) {
+            // Preserve record-relative links, but never accept other schemes or
+            // protocol-relative URLs, including ones containing "http" later on.
+            if (/^[a-z][a-z\d+.-]*:|^[/\\]{2}/i.test(url.trim())) {
+              return keepHtml(escapeHTML(match));
+            }
+            url = `${window.wpApiShare.site_url}/${window.wpApiShare.post_type}/${url}`;
+          }
+          return keepHtml(
+            isHttpUrl(url)
+              ? `<a href="${escapeHTML(url)}">${formatEmphasis(escapeHTML(text))}</a>`
+              : escapeHTML(match),
+          );
+        },
       );
-      let mentionRegex = /\@\[(.*?)\]\((.+?)\)/g;
-      comment = comment.replace(mentionRegex, (match, text, id) => {
-        /* dir=auto means that @ will be put to the left of the name if the
-         * mentioned name is LTR, and to the right if the mentioned name is
-         * RTL, instead of letting the surrounding dir determine the placement
-         * of @ */
-        return `<a dir="auto">@${text}</a>`;
-      });
+
       let urlRegex =
         /((href=('|"))|(\[|\()?|(http(s)?:((\/)|(\\))*.))*(www\.)?[-a-zA-Z0-9@:%._\+~#=]{2,256}\.[a-z]{2,6}\b([-a-zA-Z0-9@:%_\+.~#?&//\\=]*)/g;
 
@@ -798,31 +855,17 @@ window.SHAREDFUNCTIONS = {
           } else if (match.indexOf('http') === -1) {
             url = 'http://' + match;
           }
-          if (comment.includes('<img')) {
-            //do nothing
-          } else {
-            return `<a href="${url}" rel="noopener noreferrer" target="_blank">${match}</a>`;
-          }
+          return keepHtml(
+            isHttpUrl(url)
+              ? `<a href="${escapeHTML(url)}" rel="noopener noreferrer" target="_blank">${escapeHTML(match)}</a>`
+              : escapeHTML(match),
+          );
         }
         return match;
       });
-      let linkRegex = /\[(.*?)\]\((.+?)\)/g; //format [text](link)
-      comment = comment.replace(linkRegex, (match, text, url) => {
-        if (text.includes('http') && !url.includes('http')) {
-          [url, text] = [text, url];
-        }
-        url = url.includes('http')
-          ? url
-          : `${window.wpApiShare.site_url}/${window.wpApiShare.post_type}/${url}`;
-        return `<a href="${url}">${text}</a>`;
-      });
 
-      //**comment** becomes <strong>comment</strong> */
-      comment = comment.replace(/\*\*(.*?)\*\*/g, '<strong>$1</strong>');
-      //__comment__ becomes <strong>comment</strong> */
-      comment = comment.replace(/\_\_(.*?)\_\_/g, '<strong>$1</strong>');
-      //*comment* becomes <em>comment</em> */
-      comment = comment.replace(/\*(.*?)\*/g, '<em>$1</em>');
+      // Escape all remaining text before adding the supported formatting.
+      comment = formatEmphasis(escapeHTML(comment));
       //- comment or * comment or + comment becomes <strong>comment</strong> */
       comment = comment.replace(/^\s*[*+-]\s+(.*)$/gm, '<li>$1</li>');
       // also, #. becomes a numbered list
@@ -867,7 +910,10 @@ window.SHAREDFUNCTIONS = {
       }
 
       //return modified comment
-      return comment;
+      return comment
+        .split('\u0000')
+        .map((part, index) => (index % 2 ? html[part] : part))
+        .join('');
     }
 
     return comment;

package.json

@@ -42,6 +42,7 @@
     "not IE 11"
   ],
   "scripts": {
+    "test:js": "node --test tests/js/*.test.js",
     "postinstall": "npm run build",
     "build:scripts": "vite build --config vite.config.scripts.js",
     "build": "vite build",

tests/README.md

@@ -1,5 +1,8 @@
 # Running Tests
 
+Run the JavaScript helper regression tests with `npm run test:js` (Node.js 22).
+These tests do not require a running WordPress site.
+
 See [PHPUnit Docs](https://phpunit.readthedocs.io/en/9.5/textui.html) for all command line options
 
 ## Test Output