From 11daee6d4fc6bfcc5f017e01a6780a4e96ff4b14 Mon Sep 17 00:00:00 2001 From: corsac Date: Fri, 23 Oct 2026 16:34:19 +0100 Subject: [PATCH 1/2] Fix comment markdown HTML injection (GO-39) --- .github/workflows/ci.yml | 3 + dt-assets/js/activity-log.js | 6 +- dt-assets/js/comments.js | 23 +-- dt-assets/js/shared-functions.js | 105 +++++++++----- package.json | 1 + tests/README.md | 3 + tests/js/format-comment.test.js | 236 +++++++++++++++++++++++++++++++ 7 files changed, 324 insertions(+), 53 deletions(-) create mode 100644 tests/js/format-comment.test.js diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 57f2f16b6b..bf0a4b5d53 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/dt-assets/js/activity-log.js b/dt-assets/js/activity-log.js index 4ef504afbe..c75686ba68 100644 --- a/dt-assets/js/activity-log.js +++ b/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 += `
  • ${newObjectNote}
    • `; }); diff --git a/dt-assets/js/comments.js b/dt-assets/js/comments.js index d52c25ad2e..15d35c25f1 100644 --- a/dt-assets/js/comments.js +++ b/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 "
    Hello" gets transformed to "
    Hello
    ", 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 = $('
    ').text(comment.comment_content).text(); - if (!typesCount[comment.comment_type]) { typesCount[comment.comment_type] = 0; } diff --git a/dt-assets/js/shared-functions.js b/dt-assets/js/shared-functions.js index e760a4fc33..22dafc57a8 100644 --- a/dt-assets/js/shared-functions.js +++ b/dt-assets/js/shared-functions.js @@ -769,19 +769,69 @@ window.SHAREDFUNCTIONS = { */ formatComment(comment) { if (comment) { - //![image](imageUrl) becomes image */ + const escapeHTML = window.SHAREDFUNCTIONS.escapeHTML; + const formatEmphasis = (text) => + text + .replace(/\*\*(.*?)\*\*/g, '$1') + .replace(/__(.*?)__/g, '$1') + .replace(/\*(.*?)\*/g, '$1'); + 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, - "$1", + /([!@]?)\[(.*?)\]\((.*?)\)/g, + (match, type, text, url) => { + if (type === '!') { + return keepHtml( + isHttpUrl(url) + ? `${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( + `@${formatEmphasis(escapeHTML(text))}`, + ); + } + 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) + ? `${formatEmphasis(escapeHTML(text))}` + : 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 `@${text}`; - }); + let urlRegex = /((href=('|"))|(\[|\()?|(http(s)?:((\/)|(\\))*.))*(www\.)?[-a-zA-Z0-9@:%._\+~#=]{2,256}\.[a-z]{2,6}\b([-a-zA-Z0-9@:%_\+.~#?&//\\=]*)/g; @@ -798,31 +848,17 @@ window.SHAREDFUNCTIONS = { } else if (match.indexOf('http') === -1) { url = 'http://' + match; } - if (comment.includes('${match}`; - } + return keepHtml( + isHttpUrl(url) + ? `${escapeHTML(match)}` + : 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 `${text}`; - }); - //**comment** becomes comment */ - comment = comment.replace(/\*\*(.*?)\*\*/g, '$1'); - //__comment__ becomes comment */ - comment = comment.replace(/\_\_(.*?)\_\_/g, '$1'); - //*comment* becomes comment */ - comment = comment.replace(/\*(.*?)\*/g, '$1'); + // Escape all remaining text before adding the supported formatting. + comment = formatEmphasis(escapeHTML(comment)); //- comment or * comment or + comment becomes comment */ comment = comment.replace(/^\s*[*+-]\s+(.*)$/gm, '
  • $1
    • '); // also, #. becomes a numbered list @@ -867,7 +903,10 @@ window.SHAREDFUNCTIONS = { } //return modified comment - return comment; + return comment + .split('\u0000') + .map((part, index) => (index % 2 ? html[part] : part)) + .join(''); } return comment; diff --git a/package.json b/package.json index 5dfd858a4a..78cb697898 100644 --- a/package.json +++ b/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", diff --git a/tests/README.md b/tests/README.md index 6e2bba85b1..d5ea21372c 100644 --- a/tests/README.md +++ b/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 diff --git a/tests/js/format-comment.test.js b/tests/js/format-comment.test.js new file mode 100644 index 0000000000..709339b5e1 --- /dev/null +++ b/tests/js/format-comment.test.js @@ -0,0 +1,236 @@ +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import test from 'node:test'; +import vm from 'node:vm'; + +// Load the production helpers without running page initialization or requiring +// WordPress, jQuery, or a live site. +const source = readFileSync( + new URL('../../dt-assets/js/shared-functions.js', import.meta.url), + 'utf8', +); +const context = vm.createContext({ + URL, + window: { + wpApiShare: { site_url: 'https://dt.example/site', post_type: 'contacts' }, + }, +}); +vm.runInContext( + source.slice( + source.indexOf('window.SHAREDFUNCTIONS = {'), + source.indexOf('\nlet date_ranges ='), + ), + context, +); +const { formatComment, escapeHTML } = context.window.SHAREDFUNCTIONS; + +test('escapes plain text and preserves literal entities without decoding them', () => { + const text = ` & 'quotes' " &`; + assert.equal(formatComment(text), escapeHTML(text)); +}); + +test('escapes image URLs and alt text at their attribute boundaries', () => { + const url = `https://example.org/image.png?' onerror='alert&x="y"`; + const alt = `portrait' onload='alert & "name"`; + assert.equal( + formatComment(`![${alt}](${url})`), + `${escapeHTML(alt)}`, + ); +}); + +test('escapes markdown link destinations and labels', () => { + const url = 'https://example.org/" onmouseover="alert?x=1&y=2'; + const label = ` O'Brien & friends`; + assert.equal( + formatComment(`[${label}](${url})`), + `${escapeHTML(label)}`, + ); +}); + +test('leaves unsafe and malformed image/link destinations as escaped text', () => { + for (const url of [ + 'javascript:alert', + 'javascript:alert//https://example.org', + 'data:text/html,', + 'vbscript:alert', + 'file:///tmp/image.png', + '//example.org/image.png', + 'https://', + ]) { + for (const prefix of ['!', '']) { + const input = `${prefix}[label](${url})`; + assert.equal(formatComment(input), escapeHTML(input), input); + } + } +}); + +test('preserves HTTP and HTTPS images, external links, and record links', () => { + assert.equal( + formatComment('![photo](http://example.org/photo.png)'), + 'photo', + ); + assert.equal( + formatComment('[Website](https://example.org/?a=1&b=2)'), + 'Website', + ); + assert.equal( + formatComment('[Alice](123)'), + 'Alice', + ); + assert.equal( + formatComment('[https://example.org](Website)'), + 'Website', + ); +}); + +test('preserves multilingual mentions and emphasis while escaping their text', () => { + assert.equal( + formatComment('@[**علي** & ](42)'), + '@علي & <name>', + ); + assert.equal( + formatComment('**bold** __also bold__ *italic*'), + 'bold also bold italic', + ); + assert.equal( + formatComment('[**Website**](https://example.org)'), + 'Website', + ); +}); + +test('does not reparse generated attributes or URLs as markdown', () => { + const url = 'https://example.org/**path**?q=__value__'; + assert.equal( + formatComment(`![**literal alt**](${url}) [link](${url})`), + `**literal alt** link`, + ); +}); + +test('autolinks domains and query strings, leaves email alone, and works beside images', () => { + assert.equal( + formatComment('https://example.org/?a=1&b=2'), + 'https://example.org/?a=1&b=2', + ); + assert.equal(formatComment('alice@example.org'), 'alice@example.org'); + assert.equal( + formatComment('![photo](https://example.org/photo.png) www.example.org'), + 'photo www.example.org', + ); +}); + +test('preserves list formatting and escapes list contents', () => { + assert.match( + formatComment('- **first**\n- '), + /^
    • first<\/strong><\/li>\n
    • <unsafe>/, + ); + assert.match(formatComment('1. first\n2. second'), /^
      1. 1\. first/); +}); + +test('keeps input from impersonating generated markup placeholders', () => { + assert.equal( + formatComment('\u00000\u0000 [safe](https://example.org)'), + '0 safe', + ); +}); + +test('preserves empty input and can repeatedly render the same raw activity note', () => { + for (const input of ['', undefined, null]) { + assert.equal(formatComment(input), input); + } + const note = "Changed to **O'Brien & sons**"; + const expected = 'Changed to O'Brien & sons'; + assert.equal(formatComment(note), expected); + assert.equal(formatComment(note), expected); +}); + +test('record activity can rerender without mutating or double escaping its source', () => { + const commentsSource = readFileSync( + new URL('../../dt-assets/js/comments.js', import.meta.url), + 'utf8', + ); + const start = commentsSource.indexOf(' function display_activity_comment()'); + const end = commentsSource.indexOf('\n function emojiButtons()', start); + const date = { diff: () => 0 }; + const rawNote = "Updated **O'Brien & sons**: [Alice](123) {123}"; + const activity = Object.freeze({ object_note: rawNote, date, name: 'Admin' }); + const rendered = []; + const wrapper = { + empty: () => { + rendered.length = 0; + }, + append: (value) => rendered.push(value), + }; + context.window.SHAREDFUNCTIONS.getCookie = () => '[]'; + context.window.SHAREDFUNCTIONS.formatDate = () => 'Date'; + context.window.lodash = { + union: (first, second) => [...new Set([...first, ...second])], + first: (array) => array[0], + }; + const displayContext = vm.createContext({ + window: context.window, + document: { querySelectorAll: () => [] }, + $: () => wrapper, + moment: () => ({ unix: () => 0 }), + activity: [activity], + comments: [{ comment_content: 'A & B', date, comment_author: 'Admin' }], + commentsSettings: {}, + formatComment, + formatTimestampToDate: () => 'Date', + formatTimestampToDateTime: () => 'Date and time', + commentTemplate: (data) => data, + }); + vm.runInContext( + `'use strict';\n${commentsSource.slice(start, end)}`, + displayContext, + ); + for (let i = 0; i < 2; i++) { + vm.runInContext('display_activity_comment()', displayContext); + assert.equal(activity.object_note, rawNote); + assert.equal( + rendered[0].activity.find((item) => !item.comment).text, + 'Updated O'Brien & sons: Alice Date', + ); + assert.equal( + rendered[0].activity.find((item) => item.comment).text, + 'A & B', + ); + } +}); + +test('grouped activity comments are escaped exactly once, including dated notes', () => { + const activitySource = readFileSync( + new URL('../../dt-assets/js/activity-log.js', import.meta.url), + 'utf8', + ); + const start = activitySource.indexOf(' function makeGroupedActivitiesHtml('); + const end = activitySource.indexOf( + '\n function groupActivityByDayAndRecord(', + start, + ); + const activityContext = vm.createContext({ + window: context.window, + timestamptoDate: () => 'Date', + groupActivityTypes: ([action]) => ({ + [action]: { + fields: [], + object_notes: ["**O'Brien & sons** {123}"], + }, + }), + }); + vm.runInContext(activitySource.slice(start, end), activityContext); + const render = (action) => + vm.runInContext( + `makeGroupedActivitiesHtml(['${action}'], {more: 'More', less: 'Less'}, false)`, + activityContext, + ); + assert.ok( + render('comment').includes( + '
      2. O'Brien & sons <unsafe> Date
        3. ', + ), + ); + assert.ok( + render('other').includes( + '
      3. **O'Brien & sons** <unsafe> Date
        4. ', + ), + ); +}); From 6de888087c053973ea0a4b27ee7203998df6667b Mon Sep 17 00:00:00 2001 From: corsac Date: Fri, 23 Oct 2026 16:42:29 +0100 Subject: [PATCH 2/2] Preserve WordPress comment entities during escaping --- dt-assets/js/shared-functions.js | 9 ++++++++- tests/js/format-comment.test.js | 19 +++++++++++++++++-- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/dt-assets/js/shared-functions.js b/dt-assets/js/shared-functions.js index 22dafc57a8..e5fc74b087 100644 --- a/dt-assets/js/shared-functions.js +++ b/dt-assets/js/shared-functions.js @@ -769,7 +769,14 @@ window.SHAREDFUNCTIONS = { */ formatComment(comment) { if (comment) { - const escapeHTML = window.SHAREDFUNCTIONS.escapeHTML; + // 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. "&" after rendering. + const escapeHTML = (value) => + window.SHAREDFUNCTIONS.escapeHTML(value).replace( + /&((?:#\d+|#x[\da-f]+|[a-z][a-z\d]+);)/gi, + '&$1', + ); const formatEmphasis = (text) => text .replace(/\*\*(.*?)\*\*/g, '$1') diff --git a/tests/js/format-comment.test.js b/tests/js/format-comment.test.js index 709339b5e1..2d66b0e1e6 100644 --- a/tests/js/format-comment.test.js +++ b/tests/js/format-comment.test.js @@ -24,9 +24,24 @@ vm.runInContext( ); const { formatComment, escapeHTML } = context.window.SHAREDFUNCTIONS; -test('escapes plain text and preserves literal entities without decoding them', () => { +test('escapes raw markup without double encoding WordPress entities', () => { const text = ` & 'quotes' " &`; - assert.equal(formatComment(text), escapeHTML(text)); + assert.equal( + formatComment(text), + '<img src=x onerror="alert(1)"> & 'quotes' " &', + ); + assert.equal( + formatComment('Tom & Jerry &lt;tag&gt;'), + 'Tom & Jerry &lt;tag&gt;', + ); + assert.equal( + formatComment('[Tom & Jerry](https://example.org/?a=1&b=2)'), + 'Tom & Jerry', + ); + assert.equal( + formatComment('![" onload=marker](https://example.org/?x=')'), + '" onload=marker', + ); }); test('escapes image URLs and alt text at their attribute boundaries', () => {